tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: DRM/KMS: vmwgfx driver is now available
On 7/22/23 22:23, Taylor R Campbell wrote:
Date: Sun, 16 Jul 2023 04:50:35 +0900
From: PHO <pho%cielonegro.org@localhost>
On 7/16/23 04:19, Taylor R Campbell wrote:
1. Did you reimplement an allocator for the dma_pool_* API? Should it
use vmem(9) instead or can that not handle the requests that it
needs?
Yes I reimplemented it but it's probably far from efficient. And I
didn't know the existence of vmem! Lol, I should have used it.
Could I talk you into converting it to use vmem(9), so we have fewer
allocators to debug? Should be pretty easy, there's a couple other
examples of vmem(9) in drm already: drm_vma_manager.c,
drm_gem_cma_helper.c.
Yeah, I will give it a try.
For that matter, do we even need a new allocator here? Can we just do
a new bus_dmamem_alloc/load for each one?
Sure, but I fear that would degrade the performance.
Or, how important is this API? It looks like there are two calls to
dma_pool_zalloc in vmwgfx. Maybe it would be simpler to patch them
away so we don't have to spend a lot of time matching all the
semantics -- and tracking any changes in updates later -- for exactly
two users.
Which API? vmw_cmdbuf? It's a command buffer used for sending commands
to the virtual GPU, so we can't get rid of it.
2. What's up with the restore_mode property? Does anything set it?
What's the protocol? Why isn't this needed by all the other
drivers, which can already gracefully handle exiting X?
vmwgfxfb sets it and vmwgfx uses it. It's not needed as long as X
gracefully exits, because it calls WSDISPLAYIO_SMODE ioctl to revert the
mode back to WSDISPLAYIO_MODE_EMUL. But if X crashes or is killed by
SIGKILL, the only way to perform a cleanup is to do something in the
drm_driver::lastclose callback. That's why.
But why isn't this needed by all the other drivers?
I don't know. I suspect they have the same issue I explained because
their lastclose() looks like a no-op, but I can't test any other drivers
because I currently don't own any machines that runs NetBSD natively as
opposed to a VMware guest.
3. Why all the logic in vmwgfxfb.c duplicating genfb? Why not just
use genfb via drmfb, like the other major drm drivers use (i915,
radeon, nouveau, amdgpu)?
Because vmwgfx does not, and can not use drm_fb_helper. The helper
assumes that you can just allocate a framebuffer in VRAM and map it to a
virtual address. The VMware GPU appears to support this operation mode
too, but the vmwgfx driver isn't capable of doing it. So I had to
allocate a framebuffer in the system memory and ask the driver to upload
dirty areas to the GPU every time something changes in the buffer.
What a nightmare!
Is there a way we could just use or add a simple hook in genfb or
rasops for when there's dirty areas to update, so we don't have to
copy & paste all of that cruft from genfb and drmfb?
It looks like a painful maintenance burden; maintaining genfb on its
own is bad enough, and there's a lot of API cleanup warranted in
genfb, rasops, and wscons.
Yeah it is. I think we can extend the genfb API to do it and make
vmwgfxfb use it. I'll see what I can do.
- In:
/* Pretend we just completed a DMA. The device sometimes updates
* the status field synchronously without an IRQ, and we need our
* CPU to realize that. */
Seems suspicious. Do you mean that header->cb_header->status gets
updated but the device doesn't subsequently deliver an interrupt?
Yes, exactly that. This weird device really does it.
- In:
#if defined(__NetBSD__)
/* XXX: This is the only mode we support at the moment. The other
* modes will cause the driver to panic() because we don't have
* ttm_pool_populate(). */
dev_priv->map_mode = vmw_dma_alloc_coherent;
Can you make vmw_ttm_populate use ttm_bus_dma_populate more or less
unconditionally like all the other drivers do?
Okay, I'll do it.
vmwgfx_drv.h:
- Why is vmw_ttm_fault_reserve_notify here? Why isn't it static to
vmwgfx_ttm_buffer.c?
Can't remember why I did it. I'll move the function back to
vmwgfx_ttm_buffer.c and make it static.
- In:
dev->driver->irq_handler = vmw_irq_handler;
Why is this not statically initialized? dev->driver should really
be const so the compiler rejects this.
Can't remember why. Probably because vmw_irc_handler() was a static
function? I'll fix it.
vmwgfx_page_dirty.c:
- `const struct ttm_buffer_object *bo',
not `struct ttm_buffer_object const* bo'
- In:
return ttm_bo_uvm_fault(ufi, vaddr, pps, npages, centeridx,
access_type, flags);
Do we eve have to define vmw_bo_vm_fault? Let's just set
vmw_uvm_ops.pgo_fault = ttm_bo_uvm_fault, to keep it simpler?
Nope, we don't need to. I'll do it.
vmwgfx_resource.c:
- In:
/* This looks wrong, doesn't it? But it's actually what the
* original Linux code does. The tree is expected to be able to
* hold multiple nodes with the same index. */
if (a->backup_offset < b->backup_offset)
return -1;
else
return +1;
I suggest making this a total order by picking some other
distinguishing characteristic; otherwise rbtree(3) might get
confused. If nothing else, you can use (uintptr_t)a and
(uintptr_t)b (but preferably something else so it doesn't depend on
kernel virtual address ordering).
Okay I'll try something like that.
- In:
static dma_addr_t __vmw_piter_dma_addr(struct vmw_piter *viter)
{
#if defined(__NetBSD__)
/* XXX: Is this... really? Really what we need to do? */
struct vm_page* const page = &viter->pages[viter->i]->p_vmp;
paddr_t const paddr = VM_PAGE_TO_PHYS(page);
bus_addr_t const baddr = PHYS_TO_BUS_MEM(viter->dmat, paddr);
return baddr;
Seems wrong -- surely there should be a bus dma map here to use?
Yes there is, but bus_dmamap_t isn't page-oriented, right? I don't know
how to iterate through pages with it.
vmwgfx_validation.c:
- In:
#if defined(__NetBSD__)
/* vmw_validation_res_node::private is defined as "unsigned long
* private[0]". The struct has essentially a variable length. I believe
* it's a wrong way to do it, and our <libkern.h> dislikes type-mismatched
* use of container_of() against it. */
# define __LGTM_BOT__ 1
#endif
Maybe just patch the member to be `void *private[]' instead of
`unsigned long private[0]'?
Yeah that would be better. I'll do it.
- Instead of:
#if defined(__NetBSD__)
# include "vmwgfx_page.h"
/* Rewrite "struct page" to "struct vmw_page". Can't do it with a typedef. */
# define page vmw_page
static inline struct vmw_page*
alloc_page(gfp_t gfp) {
return vmw_alloc_page(gfp);
}
I suggest just patching the one call to alloc_page/__free_page.
Trying to mimic the semantics of Linux's page allocator is likely to
be a worse maintenance burden than just patching the one call here,
especially if Linux expects any guaranteed correspondence between
pages and DMA addresses. I see only 2-3 references to `struct page'
in the file, and both in local variables, so it's probably not that
big a deal to just patch it to use uvm_km here.
Alright, I'll do it.
linux_dma_fence.c:
- In:
- KASSERT(dma_fence_referenced_p(fence));
+ dma_fence_assert_alive(fence);
These are not the same -- why the change?
Yeah this one... I know they aren't the same and you won't like it (_I
don't_) but there's a reason why the change is needed:
https://github.com/NetBSD/src/commit/af88505b56f338c25163dd3f172394b9f2a86492
--
linux/dma-fence.h: Allow fences to be signaled after their refcount
going down to zero as long as their .release callback hasn't called
dma_fence_free() yet. This semantics is not documented anywhere, and is
probably just an implementation detail, but the vmwgfx driver heavily
relies on it.
Sorry riastradh@, this is unfortunate but circumventing this (admittedly
healthy) assertion requires a major code reorganization of the driver.
We really can't fight against the upstream on this matter.
--
linux_sgt.c:
- In:
bus_addr_t const baddr = PHYS_TO_BUS_MEM(piter->sg->sg_dmat, paddr);
I don't think this can be right, although it might work by acident
with vmwgfx. What's this trying to accomplish? Generally nothing
should ever use PHYS_TO_BUS_MEM outside the implementation of
bus_dma(9) to populate a bus dma map. Page addresses are not
guaranteed to have any corresponding bus dma map.
I honestly don't fully understand why vmwgfx wants to translate struct
page* into bus_addr_t, but I think I can get rid of it. Seems like it's
an unused code path as long as we stick to the vmw_dma_alloc_coherent mode.
Home |
Main Index |
Thread Index |
Old Index