Hi Am 01.12.20 um 11:00 schrieb Daniel Vetter:
[...] For my POV, the current interfaces have no clear policy or semantics. Looking through the different GEM implementations, each one seems to have its own interpretation.Yup, that's the problem really. In the past we've had vmap exclusively for permanently pinned access, with no locking requirements. Now we're trying to make this more dynamic, but in a somewhat ad-hoc fashion (generic fbdev emulation charged ahead with making the fbdev framebuffer evictable), and now it breaks at every seam. Adding more ad-hoc semantics on top doesn't seem like a good idea. That's why I think we should have 2 different interfaces: - dma_buf_vmap, the one we currently have. Permanently pins the buffer, mapping survives, no locking required.
I just looked at the implementation of dma_buf_vmap() and there's no pinning happening AFAICT. Also, none of the callback's implementations does pinning (except vram helpers). Do you mean dma_buf_attach() instead?
Best regards Thomas
- dma_buf_vmap_local, the new interface, the one that generic fbdev should have used (but oh well mistakes happen), requires dma_resv_lock, the mapping is only local to the caller Trying to shovel both semantics into one interface, depending upon which implementation we have backing the buffer, doesn't work indeed. Also on the pin topic, I think neither interface should require callers to explicitly pin anything. For existing users it should happen automatically behind the scenes imo, that's what they're expecting. -DanielI think we could use what we've done for dynamic dma-buf attachment (which also change locking rules) and just have new functions for the new way (i.e. short term vmap protected by dma_resv lock. Maybe call these dma_buf_vmap_local, in the spirit of the new kmap_local which are currently under discussion. I think _local suffix is better, for otherwise people might do something silly like dma_resv_lock(); dma_buf_vmap_locked(); dma_resv_unlock(); /* actual access maybe even in some other thread */ dma_buf_resv_lock(); dma_buf_vunmap_unlocked(); dma_resv_unlock(); _local suffix is better at telling that the resulting pointer has very limited use (essentially just local to the calling context, if you don't change any locking or anything)._local sounds good. Best regards ThomasI think encouraging importers to call dma_buf_pin/unpin isn't a good idea. Yes dynamic ones need it, but maybe we should check for that somehow in the exporterd interface (atm only amdgpu is using it). -DanielBest regards ThomasCheers, Christian.That's what I meant with that this approach here is very sprawling :-/ -Daniel*/ int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map) diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 5e6daa1c982f..7c34cd5ec261 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -137,7 +137,21 @@ struct drm_gem_object_funcs { * Returns a virtual address for the buffer. Used by the * drm_gem_dmabuf_vmap() helper. * + * Notes to implementors: + * + * - Implementations must expect pairs of @vmap and @vunmap to be + * called frequently and should optimize for this case. + * + * - Implemenations may expect the caller to hold the GEM object's + * reservation lock to protect against concurrent calls and relocation + * of the GEM object. + * + * - Implementations may provide additional guarantees (e.g., working + * without holding the reservation lock). + * * This callback is optional. + * + * See also drm_gem_dmabuf_vmap() */ int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map); @@ -148,6 +162,8 @@ struct drm_gem_object_funcs { * drm_gem_dmabuf_vunmap() helper. * * This callback is optional. + * + * See also @vmap. */ void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map); -- 2.29.2_______________________________________________ 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
