On Fri, Dec 04, 2020 at 09:47:08AM +0100, Christian König wrote: > Am 04.12.20 um 09:32 schrieb Thomas Zimmermann: > > Hi > > > > Am 03.12.20 um 21:41 schrieb Daniel Vetter: > > > On Thu, Dec 03, 2020 at 07:59:04PM +0100, Thomas Zimmermann wrote: > > > > Hi > > > > > > > > Am 03.12.20 um 16:26 schrieb Daniel Vetter: > > > > > On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote: > > > > > > Dma-buf's vmap and vunmap callbacks are undocumented and various > > > > > > exporters currently have slightly different semantics for them. Add > > > > > > documentation on how to implement and use these interfaces correctly. > > > > > > > > > > > > v2: > > > > > > * document vmap semantics in struct dma_buf_ops > > > > > > * add TODO item for reviewing and maybe fixing dma-buf exporters > > > > > > > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> > > > > > > > > > > I still don't think this works, we're breaking dma_buf_vmap > > > > > for everyone > > > > > else here. > > > > > > > > I removed the text on the importer. These notes for callers in > > > > the docs are > > > > more or less a consequence of the exporter semantics. > > > > > > Callers are importers, so I'm not seeing how that fixes anything. > > > > > > > I thought we at least agreed on the exporter semantics in the > > > > other thread, > > > > didn't we? > > > > > > > > What I'm trying to do is to write dome some rules for exporters, > > > > even if not > > > > all exporters follow them. > > > > > > This is a standard interface, everyone needs to follow the same > > > rules. And > > > if they change, we need to make sure nothing breaks and we're not > > > creating > > > issues. > > > > > > In the past the rule was the dma_buf_vmap was allowed to take the > > > dma_resv_lock, and that the buffer should be pinned. Now some ttm > > > drivers > > > didn't ever bother with the pinning, and mostly got away with that > > > because > > > drm_prime helpers do the pinning by default at attach time, and most > > > users > > > do call dma_buf_attach. > > > > > > But if you look through dma-buf docs nothing ever said you have to do a > > > dummy attachment before you call dma_buf_vmap, that's just slightly > > > crappy > > > implementations that didn't blow up yet. > > > > I had a patch for adding pin to radeon's implementation of vmap. [1] > > Christian told me to not do this; instead just get the lock in the fbdev > > code. His advise almost seems the opposite of what you're telling me > > here. > > I think what Daniel suggests here is that we need to smoothly transition the > code from making assumptions to having a straight interface where importers > explicitly say when stuff is locked and when stuff is pinned. > > I've started this with the attach interface by adding a new dynamic approach > to that, but you probably need to carry on here with that for vmap as well. > > When that is done we can migrate every exporter over to the new dynamic > approach. > > > > > For the GEM VRAM helpers, that implicit pin in vmap gave me headaches. > > Because scanouts can only be done from VRAM, which is badly suited for > > exporting. So I ended up with an implicit pin that pins the buffer to > > whatever domain it currently is. I got away with it because GEM VRAM BOs > > are not sharable among devices; fbdev is the only user of that > > functionality and only pins for short periods of time. > > > > I suspect that fixing TTM-based drivers by adding an implicit pin would > > result in a similar situation. Whatever domain it ends up pinning, some > > functionality might not be compatible with that. > > Correct, exactly that's the problem. > > > > > > > > > > Given the circumstances, we should leave out this patch from the > > > > patchset. > > > > > > So the defacto rules are already a big mess, but that's not a good > > > excuse > > > to make it worse. > > > > > > What I had in mind is that we split dma_buf_vmap up into two variants: > > > > > > - The current one, which should guarantee that the buffer is pinned. > > > Because that's what all current callers wanted, before the fbdev code > > > started allowing non-pinned buffers. > > > > Can we add an explicit pin operation to dma_buf_vmap() to enforce the > > semantics? > > At least I would be fine with that. For now amdgpu is the only exporter who > implements the explicit pin/unpin semantics anyway. Yup, I think that makes sense (if it works). Maybe we could use something like: a) dma_buf pin exists, driver is dynamic. This means dma_buf_vmap needs to first pin, then call ->vmap. dma_buf_vmap_local otoh can directly call ->vmap since the exporter relies on either a pin or dma_resv_lock. b) dma_buf pin not implement, driver is a legacy pile. dma_buf_vmap will pin (somewhere at least, or rely on some implicit pin), dma_buf_vmap_local doesn't work and should fail. I think for less transition work fbdev helpers could first try dma_resv_lock + dma_buf_vmap_local, if that fails, drop the dma_resv_lock and do the pinning dma_buf_vmap. That way we don't have to convert shmem helpers over to dma_resv locking, which should help. And ttm drivers would do the new clean interface, so at least everyone using dma_resv today is all fine. Intel's conversion to dma_resv lock is in-flight, but that needs a conversion to the dynamic interface anyway, the current code splats. And dynamic brings means explicit pin/unpin callbacks, so should be good too. -Daniel > > Regards, > Christian. > > > > > Best regards > > Thomas > > > > [1] https://patchwork.freedesktop.org/patch/400054/?series=83765&rev=1 > > > > > > > > - The new one, which allows vmapping with just dma_resv locked, and > > > should > > > have some caching in exporters. > > > > > > Breaking code and then adding todos about that is kinda not so cool > > > approach here imo. > > > > > > Also I guess ttm_bo_vmap should have a check that either the buffer is > > > pinned, or dma_resv_lock is held. > > > > > > Cheers, Daniel > > > > > > > > > > > > > > > > > Best regards > > > > Thomas > > > > > > > > > > > > > > > --- > > > > > > Documentation/gpu/todo.rst | 15 +++++++++++++ > > > > > > include/drm/drm_gem.h | 4 ++++ > > > > > > include/linux/dma-buf.h | 45 > > > > > > ++++++++++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 64 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > > > > > index 009d8e6c7e3c..32bb797a84fc 100644 > > > > > > --- a/Documentation/gpu/todo.rst > > > > > > +++ b/Documentation/gpu/todo.rst > > > > > > @@ -505,6 +505,21 @@ Contact: Thomas Zimmermann > > > > > > <tzimmermann@xxxxxxx>, Christian König, Daniel Vette > > > > > > Level: Intermediate > > > > > > +Enforce rules for dma-buf vmap and pin ops > > > > > > +------------------------------------------ > > > > > > + > > > > > > +Exporter implementations of vmap and pin in struct > > > > > > dma_buf_ops (and consequently > > > > > > +struct drm_gem_object_funcs) use a variety of locking > > > > > > semantics. Some rely on > > > > > > +the caller holding the dma-buf's reservation lock, some > > > > > > do their own locking, > > > > > > +some don't require any locking. VRAM helpers even used > > > > > > to pin as part of vmap. > > > > > > + > > > > > > +We need to review each exporter and enforce the documented rules. > > > > > > + > > > > > > +Contact: Christian König, Daniel Vetter, Thomas > > > > > > Zimmermann <tzimmermann@xxxxxxx> > > > > > > + > > > > > > +Level: Advanced > > > > > > + > > > > > > + > > > > > > Core refactorings > > > > > > ================= > > > > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > > > > > index 5e6daa1c982f..1864c6a721b1 100644 > > > > > > --- a/include/drm/drm_gem.h > > > > > > +++ b/include/drm/drm_gem.h > > > > > > @@ -138,6 +138,8 @@ struct drm_gem_object_funcs { > > > > > > * drm_gem_dmabuf_vmap() helper. > > > > > > * > > > > > > * This callback is optional. > > > > > > + * > > > > > > + * See also struct dma_buf_ops.vmap > > > > > > */ > > > > > > int (*vmap)(struct drm_gem_object *obj, struct > > > > > > dma_buf_map *map); > > > > > > @@ -148,6 +150,8 @@ struct drm_gem_object_funcs { > > > > > > * drm_gem_dmabuf_vunmap() helper. > > > > > > * > > > > > > * This callback is optional. > > > > > > + * > > > > > > + * See also struct dma_buf_ops.vunmap > > > > > > */ > > > > > > void (*vunmap)(struct drm_gem_object *obj, struct > > > > > > dma_buf_map *map); > > > > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > > > > > index cf72699cb2bc..dc81fdc01dda 100644 > > > > > > --- a/include/linux/dma-buf.h > > > > > > +++ b/include/linux/dma-buf.h > > > > > > @@ -267,7 +267,52 @@ struct dma_buf_ops { > > > > > > */ > > > > > > int (*mmap)(struct dma_buf *, struct vm_area_struct *vma); > > > > > > + /** > > > > > > + * @vmap: > > > > > > > > > > There's already a @vmap and @vunamp kerneldoc at the top comment, that > > > > > needs to be removed. > > > > > -Daniel > > > > > > > > > > > + * > > > > > > + * Returns a virtual address for the buffer. > > > > > > + * > > > > > > + * Notes to callers: > > > > > > + * > > > > > > + * - Callers must hold the struct dma_buf.resv lock > > > > > > before calling > > > > > > + * this interface. > > > > > > + * > > > > > > + * - Callers must provide means to prevent the > > > > > > mappings from going > > > > > > + * stale, such as holding the reservation lock or providing a > > > > > > + * move-notify callback to the exporter. > > > > > > + * > > > > > > + * Notes to implementors: > > > > > > + * > > > > > > + * - Implementations must expect pairs of @vmap and > > > > > > @vunmap to be > > > > > > + * called frequently and should optimize for this case. > > > > > > + * > > > > > > + * - Implementations should avoid additional operations, such as > > > > > > + * pinning. > > > > > > + * > > > > > > + * - Implementations may expect the caller to hold the dma-buf's > > > > > > + * reservation lock to protect against concurrent calls and > > > > > > + * relocation. > > > > > > + * > > > > > > + * - Implementations may provide additional > > > > > > guarantees, such as working > > > > > > + * without holding the reservation lock. > > > > > > + * > > > > > > + * This callback is optional. > > > > > > + * > > > > > > + * Returns: > > > > > > + * > > > > > > + * 0 on success or a negative error code on failure. > > > > > > + */ > > > > > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > > > > > > + > > > > > > + /** > > > > > > + * @vunmap: > > > > > > + * > > > > > > + * Releases the address previously returned by @vmap. > > > > > > + * > > > > > > + * This callback is optional. > > > > > > + * > > > > > > + * See also @vmap() > > > > > > + */ > > > > > > void (*vunmap)(struct dma_buf *dmabuf, struct > > > > > > dma_buf_map *map); > > > > > > }; > > > > > > -- > > > > > > 2.29.2 > > > > > > > > > > > > > > > > > > > -- > > > > 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 > > > > > > > > > > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
