Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 26.11.20 um 13:08 schrieb Christian König:
> > [...]
> > Yeah, I remember a bug report about frequent page-table modifications 
> > wrt to vram helpers. So we implemented the lazy unmapping / vmap 
> > caching, as suggested by Christian back them. My guess is that 
> > anything TTM-based can use a similar pattern. Christian probably knows 
> > the corner cases.
>
> My memory is failing me, what corner case are you talking about?

Haha! :D What I meant was that if there were corner cases, you'd know 
about them. From your comment, I guess there are none.

Best regards
Thomas

> Christian.
>
> > CMA seems obviously working correctly already.
> >
> > For SHMEM, I'd have to figure out the reference counting of the pages 
> > involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could 
> > be moved into the free callback, plus a few other modifications.
> >
> > Best regards
> > Thomas
> >
> > > But if you're willing to do all that conversion of callers, then of
> > > course I'm not stopping you. Not at all, it's great to see that kind
> > > of maze untangled.
> > > -Daniel
> > >
> > > > Best regards
> > > > Thomas
> > > >
> > > > > That should give us at least some way forward to gradually conver 
> > > > > all the
> > > > > drivers and helpers over to dma_resv locking.
> > > > > -Daniel
> > > > >
> > > > > > The pin count is currently maintained by the vmap implementation 
> > > > > > in vram
> > > > > > helpers. Calling vmap is an implicit pin; calling vunmap is an 
> > > > > > implicit
> > > > > > unpin. This prevents eviction in the damage worker. But now I was 
> > > > > > told than
> > > > > > pinning is only for BOs that are controlled by userspace and 
> > > > > > internal users
> > > > > > should acquire the resv lock. So vram helpers have to be fixed, 
> > > > > > actually.
> > > > > >
> > > > > > In vram helpers, unmapping does not mean eviction. The unmap 
> > > > > > operation only
> > > > > > marks the BO as unmappable. The real unmap happens when the 
> > > > > > eviction takes
> > > > > > place. This avoids many modifications to the page tables. IOW an 
> > > > > > unpinned,
> > > > > > unmapped BO will remain in VRAM until the memory is actually needed.
> > > > > >
> > > > > > Best regards
> > > > > > Thomas
> > > > > >
> > > > > > > So I'm still not seeing how this can go boom.
> > > > > > >
> > > > > > > Now long term it'd be nice to cut everything over to dma_resv 
> > > > > > > locking, but
> > > > > > > the issue there is that beyond ttm, none of the helpers (and few 
> > > > > > > of the
> > > > > > > drivers) use dma_resv. So this is a fairly big uphill battle. Quick
> > > > > > > interim fix seems like the right solution to me.
> > > > > > > -Daniel
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Christian.
> > > > > > > >
> > > > > > > > > Best regards
> > > > > > > > > Thomas
> > > > > > > > >
> > > > > > > > > > > There's no recursion taking place, so I guess the reservation
> > > > > > > > > > > lock could be
> > > > > > > > > > > acquired/release in drm_client_buffer_vmap/vunmap(), or a
> > > > > > > > > > > separate pair of
> > > > > > > > > > > DRM client functions could do the locking.
> > > > > > > > > >
> > > > > > > > > > Given how this "do the right locking" is a can of worms (and I 
> > > > > > > > > > think
> > > > > > > > > > it's
> > > > > > > > > > worse than what you dug out already) I think the 
> > > > > > > > > > fb_helper.lock hack is
> > > > > > > > > > perfectly good enough.
> > > > > > > > > >
> > > > > > > > > > I'm also somewhat worried that starting to use dma_resv lock 
> > > > > > > > > > in generic
> > > > > > > > > > code, while many helpers/drivers still have their hand-rolled 
> > > > > > > > > > locking,
> > > > > > > > > > will make conversion over to dma_resv needlessly more 
> > > > > > > > > > complicated.
> > > > > > > > > > -Daniel
> > > > > > > > > >
> > > > > > > > > > > Best regards
> > > > > > > > > > > Thomas
> > > > > > > > > > >
> > > > > > > > > > > [1] 
> > > > > > > > > > > https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394 
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Please note that the reservation lock you need to take here 
> > > > > > > > > > > > is part of
> > > > > > > > > > > > the GEM object.
> > > > > > > > > > > >
> > > > > > > > > > > > Usually we design things in the way that the code needs to 
> > > > > > > > > > > > take a lock
> > > > > > > > > > > > which protects an object, then do some operations with the 
> > > > > > > > > > > > object and
> > > > > > > > > > > > then release the lock again.
> > > > > > > > > > > >
> > > > > > > > > > > > Having in the lock inside the operation can be done as well, 
> > > > > > > > > > > > but
> > > > > > > > > > > > returning with it is kind of unusual design.
> > > > > > > > > > > >
> > > > > > > > > > > > > Sorry for the noob questions. I'm still trying to 
> > > > > > > > > > > > > understand the
> > > > > > > > > > > > > implications of acquiring these locks.
> > > > > > > > > > > >
> > > > > > > > > > > > Well this is the reservation lock of the GEM object we are
> > > > > > > > > > > > talking about
> > > > > > > > > > > > here. We need to take that for a couple of different 
> > > > > > > > > > > > operations,
> > > > > > > > > > > > vmap/vunmap doesn't sound like a special case to me.
> > > > > > > > > > > >
> > > > > > > > > > > > Regards,
> > > > > > > > > > > > Christian.
> > > > > > > > > > > >
> > > > > > > > > > > > > Best regards
> > > > > > > > > > > > > Thomas
> > > > > > > > > > > >
> > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > dri-devel mailing list
> > > > > > > > > > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > Thomas Zimmermann
> > > > > > > > > > > Graphics Driver Developer
> > > > > > > > > > > SUSE Software Solutions Germany GmbH
> > > > > > > > > > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > > > > > > > > (HRB 36809, AG Nürnberg)
> > > > > > > > > > > Geschäftsführer: Felix Imendörffer
> > > > > >
> > > > > > --
> > > > > > Thomas Zimmermann
> > > > > > Graphics Driver Developer
> > > > > > SUSE Software Solutions Germany GmbH
> > > > > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > > > (HRB 36809, AG Nürnberg)
> > > > > > Geschäftsführer: Felix Imendörffer
> > > >
> > > > --
> > > > Thomas Zimmermann
> > > > Graphics Driver Developer
> > > > SUSE Software Solutions Germany GmbH
> > > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > (HRB 36809, AG Nürnberg)
> > > > Geschäftsführer: Felix Imendörffer
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: OpenPGP_0x680DC11D530B7A23.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel