> Date: Sun, 23 Jul 2023 00:47:39 +0900
> From: PHO <p...@cielonegro.org>
>
> On 7/22/23 22:23, Taylor R Campbell wrote:
> > 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.
Any idea how much? I don't have a good sense of how this is used in
the system. Is this like a network driver that precreates a pool of
mbufs and dma maps up front, and then loads and unloads them during
packet processing?
> > 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.
I meant dma_pool. I was thinking maybe we should just patch away the
uses of dma_pool in vmwgfx_cmdbuf instead of implementing the dma_pool
API. But I haven't looked that closely; what you did is probably
fine.
> >>> 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.
Seems very fishy. Suspect either:
(a) this isn't necessary, and there's something else that's different
about vmwgfx; or
(b) this is necessary for all the drivers, and we need to have a
better generic mechanism (not kludging more function pointers into
stringy device properties or device calls) for switching the genfb
mode back on lastclose.
> > 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.
Maybe macallan@ can help with getting the right hooks in here?
> > - 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.
You can ask for maxsegsz=PAGE_SIZE boundary=PAGE_SIZE in
bus_dmamap_create, and then each segment's .ds_len will be PAGE_SIZE
and each .ds_addr will be page-aligned.
> > 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.
> --
You're right that I don't like it...it is completely bonkers
use-after-free! Even i915 with its abuse of dma fence internals
doesn't violate these assertions. What is vmwgfx doing that seems to
rely on it, which isn't just a broken missing reference count
acquire/release cycle or something?
