| Message ID | 20230301222903.458692-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vram-helper: turn on PRIME import/export | expand |
Am 01.03.23 um 23:29 schrieb Simon Ser: > We don't populate gem_prime_import_sg_table so only DMA-BUFs > exported from our own device can be imported. Still, this is useful > to user-space. But what happens if one of your BOs is imported into another device? Regards, Christian. > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Tian Tao <tiantao6@hisilicon.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Christian König <christian.koenig@amd.com> > Cc: Hans de Goede <hdegoede@redhat.com> > --- > include/drm/drm_gem_vram_helper.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h > index d3e8920c0b64..f4aab64411d8 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -160,7 +160,9 @@ void drm_gem_vram_simple_display_pipe_cleanup_fb( > .debugfs_init = drm_vram_mm_debugfs_init, \ > .dumb_create = drm_gem_vram_driver_dumb_create, \ > .dumb_map_offset = drm_gem_ttm_dumb_map_offset, \ > - .gem_prime_mmap = drm_gem_prime_mmap > + .gem_prime_mmap = drm_gem_prime_mmap, \ > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle > > /* > * VRAM memory manager
Hi Am 01.03.23 um 23:29 schrieb Simon Ser: > We don't populate gem_prime_import_sg_table so only DMA-BUFs > exported from our own device can be imported. Still, this is useful > to user-space. VRAM helpers don't really offer much flexibility or control in where to place a BO. What happens if the BO is located in device video ram and cannot be used by the other participant? A blank screen? I'd also consider VRAM helpers as deprecated. The remaining few drivers can be converted to SHMEM helpers; except maybe bochs could get its own TTM-based memory management. Best regards Thomas > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Tian Tao <tiantao6@hisilicon.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Christian König <christian.koenig@amd.com> > Cc: Hans de Goede <hdegoede@redhat.com> > --- > include/drm/drm_gem_vram_helper.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h > index d3e8920c0b64..f4aab64411d8 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -160,7 +160,9 @@ void drm_gem_vram_simple_display_pipe_cleanup_fb( > .debugfs_init = drm_vram_mm_debugfs_init, \ > .dumb_create = drm_gem_vram_driver_dumb_create, \ > .dumb_map_offset = drm_gem_ttm_dumb_map_offset, \ > - .gem_prime_mmap = drm_gem_prime_mmap > + .gem_prime_mmap = drm_gem_prime_mmap, \ > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle > > /* > * VRAM memory manager
On Thursday, March 2nd, 2023 at 08:11, Christian König <christian.koenig@amd.com> wrote: > Am 01.03.23 um 23:29 schrieb Simon Ser: > > > We don't populate gem_prime_import_sg_table so only DMA-BUFs > > exported from our own device can be imported. Still, this is useful > > to user-space. > > But what happens if one of your BOs is imported into another device? Is there a way to make this fail, always?
On Thursday, March 2nd, 2023 at 09:07, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 01.03.23 um 23:29 schrieb Simon Ser: > > > We don't populate gem_prime_import_sg_table so only DMA-BUFs > > exported from our own device can be imported. Still, this is useful > > to user-space. > > VRAM helpers don't really offer much flexibility or control in where to > place a BO. That's fine. The goal here is purely export/import to the exact same device. > What happens if the BO is located in device video ram and cannot be > used by the other participant? A blank screen? A failure at import-time should happen. But maybe this patch isn't enough to make this happen?
Am 02.03.23 um 11:14 schrieb Simon Ser: > On Thursday, March 2nd, 2023 at 08:11, Christian König <christian.koenig@amd.com> wrote: > >> Am 01.03.23 um 23:29 schrieb Simon Ser: >> >>> We don't populate gem_prime_import_sg_table so only DMA-BUFs >>> exported from our own device can be imported. Still, this is useful >>> to user-space. >> But what happens if one of your BOs is imported into another device? > Is there a way to make this fail, always? Well you could return an error from the attach callback if I'm not completely mistaken. But that's certainly not a standard function of the DRM helpers. Regards, Christian.
On Thursday, March 2nd, 2023 at 14:21, Christian König <christian.koenig@amd.com> wrote: > Am 02.03.23 um 11:14 schrieb Simon Ser: > > > On Thursday, March 2nd, 2023 at 08:11, Christian König christian.koenig@amd.com wrote: > > > > > Am 01.03.23 um 23:29 schrieb Simon Ser: > > > > > > > We don't populate gem_prime_import_sg_table so only DMA-BUFs > > > > exported from our own device can be imported. Still, this is useful > > > > to user-space. > > > > But what happens if one of your BOs is imported into another device? > > > > Is there a way to make this fail, always? > > Well you could return an error from the attach callback if I'm not > completely mistaken. Hmm, but with GEM helpers this is handled by drm_gem_map_attach(). That function calls drm_gem_object_funcs.pin but doesn't pass along the dma_buf_attachment so there no way to reject the attach based on the other device there… Would it be unreasonable to add a drm_driver.gem_prime_attach hook? Or is there a better way to implement this?
Am 02.03.23 um 14:37 schrieb Simon Ser: > On Thursday, March 2nd, 2023 at 14:21, Christian König <christian.koenig@amd.com> wrote: > >> Am 02.03.23 um 11:14 schrieb Simon Ser: >> >>> On Thursday, March 2nd, 2023 at 08:11, Christian König christian.koenig@amd.com wrote: >>> >>>> Am 01.03.23 um 23:29 schrieb Simon Ser: >>>> >>>>> We don't populate gem_prime_import_sg_table so only DMA-BUFs >>>>> exported from our own device can be imported. Still, this is useful >>>>> to user-space. >>>>> But what happens if one of your BOs is imported into another device? >>>>> Is there a way to make this fail, always? >> Well you could return an error from the attach callback if I'm not >> completely mistaken. > Hmm, but with GEM helpers this is handled by drm_gem_map_attach(). That > function calls drm_gem_object_funcs.pin but doesn't pass along the > dma_buf_attachment so there no way to reject the attach based on the > other device there… > > Would it be unreasonable to add a drm_driver.gem_prime_attach hook? Or > is there a better way to implement this? That would be the mid layering we usually try hard to avoid. Your obj doesn't implement the obj->funcs->get_sg_table() callback doesn't it? In this case drm_gem_map_dma_buf() would fail just a little bit later anyway. What you could do is to add a check for the get_sg_table callback a bit earlier in drm_gem_map_attach() and if that's not implemented reject the attachment. Regards, Christian.
On Thursday, March 2nd, 2023 at 14:45, Christian König <christian.koenig@amd.com> wrote: > Am 02.03.23 um 14:37 schrieb Simon Ser: > > > On Thursday, March 2nd, 2023 at 14:21, Christian König christian.koenig@amd.com wrote: > > > > > Am 02.03.23 um 11:14 schrieb Simon Ser: > > > > > > > On Thursday, March 2nd, 2023 at 08:11, Christian König christian.koenig@amd.com wrote: > > > > > > > > > Am 01.03.23 um 23:29 schrieb Simon Ser: > > > > > > > > > > > We don't populate gem_prime_import_sg_table so only DMA-BUFs > > > > > > exported from our own device can be imported. Still, this is useful > > > > > > to user-space. > > > > > > But what happens if one of your BOs is imported into another device? > > > > > > Is there a way to make this fail, always? > > > > > > Well you could return an error from the attach callback if I'm not > > > > > > completely mistaken. > > > > > > Hmm, but with GEM helpers this is handled by drm_gem_map_attach(). That > > > > > > function calls drm_gem_object_funcs.pin but doesn't pass along the > > > > > > dma_buf_attachment so there no way to reject the attach based on the > > > > > > other device there… > > > > Would it be unreasonable to add a drm_driver.gem_prime_attach hook? Or > > is there a better way to implement this? > > That would be the mid layering we usually try hard to avoid. Indeed! > Your obj doesn't implement the obj->funcs->get_sg_table() callback > doesn't it? In this case drm_gem_map_dma_buf() would fail just a little > bit later anyway. > > What you could do is to add a check for the get_sg_table callback a bit > earlier in drm_gem_map_attach() and if that's not implemented reject the > attachment. That's a good idea, thanks for the suggestion! Done in v2.
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index d3e8920c0b64..f4aab64411d8 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -160,7 +160,9 @@ void drm_gem_vram_simple_display_pipe_cleanup_fb( .debugfs_init = drm_vram_mm_debugfs_init, \ .dumb_create = drm_gem_vram_driver_dumb_create, \ .dumb_map_offset = drm_gem_ttm_dumb_map_offset, \ - .gem_prime_mmap = drm_gem_prime_mmap + .gem_prime_mmap = drm_gem_prime_mmap, \ + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ + .prime_fd_to_handle = drm_gem_prime_fd_to_handle /* * VRAM memory manager
We don't populate gem_prime_import_sg_table so only DMA-BUFs exported from our own device can be imported. Still, this is useful to user-space. Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Tian Tao <tiantao6@hisilicon.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Christian König <christian.koenig@amd.com> Cc: Hans de Goede <hdegoede@redhat.com> --- include/drm/drm_gem_vram_helper.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)