* [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions @ 2019-06-18 13:13 Laurent Pinchart 2019-06-18 13:21 ` Daniel Vetter 2019-06-18 13:56 ` Noralf Trønnes 0 siblings, 2 replies; 9+ messages in thread From: Laurent Pinchart @ 2019-06-18 13:13 UTC (permalink / raw) To: dri-devel; +Cc: linux-renesas-soc, Daniel Vetter, Kieran Bingham The recommended way to specify GEM object functions is to provide a drm_gem_object_funcs structure instance and set the GEM object to point to it. The drm_cma_gem_create_object_default_funcs() function provided by the GEM CMA helper does so when creating the GEM object, simplifying the driver implementation. Switch to it, and remove the then unneeded GEM-related opertions from rcar_du_driver. Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) Daniel, is this what you had in mind ? diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 3e5e835ea2b6..4cbb82009931 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); static struct drm_driver rcar_du_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_ATOMIC, - .gem_free_object_unlocked = drm_gem_cma_free_object, - .gem_vm_ops = &drm_gem_cma_vm_ops, + .gem_create_object = drm_cma_gem_create_object_default_funcs, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_import = drm_gem_prime_import, - .gem_prime_export = drm_gem_prime_export, - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, - .gem_prime_vmap = drm_gem_cma_prime_vmap, - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, .gem_prime_mmap = drm_gem_cma_prime_mmap, .dumb_create = rcar_du_dumb_create, .fops = &rcar_du_fops, -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 13:13 [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions Laurent Pinchart @ 2019-06-18 13:21 ` Daniel Vetter 2019-06-18 13:26 ` Laurent Pinchart 2019-06-18 13:56 ` Noralf Trønnes 1 sibling, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2019-06-18 13:21 UTC (permalink / raw) To: Laurent Pinchart Cc: open list:DRM DRIVERS FOR RENESAS, Kieran Bingham, dri-devel On Tue, Jun 18, 2019 at 3:13 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > The recommended way to specify GEM object functions is to provide a > drm_gem_object_funcs structure instance and set the GEM object to point > to it. The drm_cma_gem_create_object_default_funcs() function provided > by the GEM CMA helper does so when creating the GEM object, simplifying > the driver implementation. Switch to it, and remove the then unneeded > GEM-related opertions from rcar_du_driver. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > Daniel, is this what you had in mind ? Yup, I think that's it. Noralf commented that we might want to have DRM_GEM_CMA_DRIVER_OPS macro for the remaining few drm_driver hooks, like DRM_GEM_CMA_VMAP_DRIVER_OPS but without the forced vmap on import. But that's ok to do in some follow-up cleanup too. On this: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > index 3e5e835ea2b6..4cbb82009931 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > static struct drm_driver rcar_du_driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > | DRIVER_ATOMIC, > - .gem_free_object_unlocked = drm_gem_cma_free_object, > - .gem_vm_ops = &drm_gem_cma_vm_ops, > + .gem_create_object = drm_cma_gem_create_object_default_funcs, > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > - .gem_prime_import = drm_gem_prime_import, > - .gem_prime_export = drm_gem_prime_export, > - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > - .gem_prime_vmap = drm_gem_cma_prime_vmap, > - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > .gem_prime_mmap = drm_gem_cma_prime_mmap, > .dumb_create = rcar_du_dumb_create, > .fops = &rcar_du_fops, > -- > Regards, > > Laurent Pinchart > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 13:21 ` Daniel Vetter @ 2019-06-18 13:26 ` Laurent Pinchart 2019-06-18 13:57 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2019-06-18 13:26 UTC (permalink / raw) To: Daniel Vetter Cc: open list:DRM DRIVERS FOR RENESAS, Laurent Pinchart, Kieran Bingham, dri-devel Hi Daniel, On Tue, Jun 18, 2019 at 03:21:55PM +0200, Daniel Vetter wrote: > On Tue, Jun 18, 2019 at 3:13 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > > > The recommended way to specify GEM object functions is to provide a > > drm_gem_object_funcs structure instance and set the GEM object to point > > to it. The drm_cma_gem_create_object_default_funcs() function provided > > by the GEM CMA helper does so when creating the GEM object, simplifying > > the driver implementation. Switch to it, and remove the then unneeded > > GEM-related opertions from rcar_du_driver. > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > Daniel, is this what you had in mind ? > > Yup, I think that's it. Noralf commented that we might want to have > DRM_GEM_CMA_DRIVER_OPS macro for the remaining few drm_driver hooks, like > DRM_GEM_CMA_VMAP_DRIVER_OPS but without the forced vmap on import. But > that's ok to do in some follow-up cleanup too. On this: Note that the rcar-du driver requires a custom .dumb_create() operation, which is another reason why I can't use DRM_GEM_CMA_VMAP_DRIVER_OPS. > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > index 3e5e835ea2b6..4cbb82009931 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > > static struct drm_driver rcar_du_driver = { > > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > > | DRIVER_ATOMIC, > > - .gem_free_object_unlocked = drm_gem_cma_free_object, > > - .gem_vm_ops = &drm_gem_cma_vm_ops, > > + .gem_create_object = drm_cma_gem_create_object_default_funcs, > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > - .gem_prime_import = drm_gem_prime_import, > > - .gem_prime_export = drm_gem_prime_export, > > - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > > .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > > - .gem_prime_vmap = drm_gem_cma_prime_vmap, > > - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > > .gem_prime_mmap = drm_gem_cma_prime_mmap, > > .dumb_create = rcar_du_dumb_create, > > .fops = &rcar_du_fops, -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 13:26 ` Laurent Pinchart @ 2019-06-18 13:57 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2019-06-18 13:57 UTC (permalink / raw) To: Laurent Pinchart Cc: Laurent Pinchart, Daniel Vetter, Kieran Bingham, dri-devel, open list:DRM DRIVERS FOR RENESAS On Tue, Jun 18, 2019 at 04:26:51PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Tue, Jun 18, 2019 at 03:21:55PM +0200, Daniel Vetter wrote: > > On Tue, Jun 18, 2019 at 3:13 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > > > > > The recommended way to specify GEM object functions is to provide a > > > drm_gem_object_funcs structure instance and set the GEM object to point > > > to it. The drm_cma_gem_create_object_default_funcs() function provided > > > by the GEM CMA helper does so when creating the GEM object, simplifying > > > the driver implementation. Switch to it, and remove the then unneeded > > > GEM-related opertions from rcar_du_driver. > > > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > > > Daniel, is this what you had in mind ? > > > > Yup, I think that's it. Noralf commented that we might want to have > > DRM_GEM_CMA_DRIVER_OPS macro for the remaining few drm_driver hooks, like > > DRM_GEM_CMA_VMAP_DRIVER_OPS but without the forced vmap on import. But > > that's ok to do in some follow-up cleanup too. On this: > > Note that the rcar-du driver requires a custom .dumb_create() operation, > which is another reason why I can't use DRM_GEM_CMA_VMAP_DRIVER_OPS. Hm I was blind. Not sure you still want my r-b :-) But yeah good reason not to use an ops macro for this. -Daniel > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > > index 3e5e835ea2b6..4cbb82009931 100644 > > > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > > @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > > > static struct drm_driver rcar_du_driver = { > > > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > > > | DRIVER_ATOMIC, > > > - .gem_free_object_unlocked = drm_gem_cma_free_object, > > > - .gem_vm_ops = &drm_gem_cma_vm_ops, > > > + .gem_create_object = drm_cma_gem_create_object_default_funcs, > > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > > - .gem_prime_import = drm_gem_prime_import, > > > - .gem_prime_export = drm_gem_prime_export, > > > - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > > > .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > > > - .gem_prime_vmap = drm_gem_cma_prime_vmap, > > > - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > > > .gem_prime_mmap = drm_gem_cma_prime_mmap, > > > .dumb_create = rcar_du_dumb_create, > > > .fops = &rcar_du_fops, > > -- > Regards, > > Laurent Pinchart -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 13:13 [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions Laurent Pinchart 2019-06-18 13:21 ` Daniel Vetter @ 2019-06-18 13:56 ` Noralf Trønnes 2019-06-18 16:35 ` Laurent Pinchart 1 sibling, 1 reply; 9+ messages in thread From: Noralf Trønnes @ 2019-06-18 13:56 UTC (permalink / raw) To: Laurent Pinchart, dri-devel Cc: linux-renesas-soc, Daniel Vetter, Kieran Bingham Den 18.06.2019 15.13, skrev Laurent Pinchart: > The recommended way to specify GEM object functions is to provide a > drm_gem_object_funcs structure instance and set the GEM object to point > to it. The drm_cma_gem_create_object_default_funcs() function provided > by the GEM CMA helper does so when creating the GEM object, simplifying > the driver implementation. Switch to it, and remove the then unneeded > GEM-related opertions from rcar_du_driver. s/opertions/operations/ > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > Daniel, is this what you had in mind ? > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > index 3e5e835ea2b6..4cbb82009931 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > static struct drm_driver rcar_du_driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > | DRIVER_ATOMIC, > - .gem_free_object_unlocked = drm_gem_cma_free_object, > - .gem_vm_ops = &drm_gem_cma_vm_ops, > + .gem_create_object = drm_cma_gem_create_object_default_funcs, > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > - .gem_prime_import = drm_gem_prime_import, > - .gem_prime_export = drm_gem_prime_export, > - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > - .gem_prime_vmap = drm_gem_cma_prime_vmap, > - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > .gem_prime_mmap = drm_gem_cma_prime_mmap, If you want to pick up yet another recommendation, you can use drm_gem_prime_mmap here. Either way: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > .dumb_create = rcar_du_dumb_create, > .fops = &rcar_du_fops, > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 13:56 ` Noralf Trønnes @ 2019-06-18 16:35 ` Laurent Pinchart 2019-06-18 17:23 ` Noralf Trønnes 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2019-06-18 16:35 UTC (permalink / raw) To: Noralf Trønnes Cc: linux-renesas-soc, Daniel Vetter, Laurent Pinchart, Kieran Bingham, dri-devel Hi Noralf, On Tue, Jun 18, 2019 at 03:56:19PM +0200, Noralf Trønnes wrote: > Den 18.06.2019 15.13, skrev Laurent Pinchart: > > The recommended way to specify GEM object functions is to provide a > > drm_gem_object_funcs structure instance and set the GEM object to point > > to it. The drm_cma_gem_create_object_default_funcs() function provided > > by the GEM CMA helper does so when creating the GEM object, simplifying > > the driver implementation. Switch to it, and remove the then unneeded > > GEM-related opertions from rcar_du_driver. > > s/opertions/operations/ Oops, will fix. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > Daniel, is this what you had in mind ? > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > index 3e5e835ea2b6..4cbb82009931 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > > static struct drm_driver rcar_du_driver = { > > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > > | DRIVER_ATOMIC, > > - .gem_free_object_unlocked = drm_gem_cma_free_object, > > - .gem_vm_ops = &drm_gem_cma_vm_ops, > > + .gem_create_object = drm_cma_gem_create_object_default_funcs, > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > - .gem_prime_import = drm_gem_prime_import, > > - .gem_prime_export = drm_gem_prime_export, > > - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > > .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > > - .gem_prime_vmap = drm_gem_cma_prime_vmap, > > - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > > .gem_prime_mmap = drm_gem_cma_prime_mmap, > > If you want to pick up yet another recommendation, you can use > drm_gem_prime_mmap here. I compared the two call stacks and they appear similar, even if drm_gem_prime_mmap() leads to a more convoluted code flow. For my information, what's the advantage in using it ? > Either way: > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > > > .dumb_create = rcar_du_dumb_create, > > .fops = &rcar_du_fops, -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 16:35 ` Laurent Pinchart @ 2019-06-18 17:23 ` Noralf Trønnes 2019-06-19 7:00 ` Noralf Trønnes 0 siblings, 1 reply; 9+ messages in thread From: Noralf Trønnes @ 2019-06-18 17:23 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-renesas-soc, Daniel Vetter, Laurent Pinchart, Kieran Bingham, dri-devel Den 18.06.2019 18.35, skrev Laurent Pinchart: > Hi Noralf, > > On Tue, Jun 18, 2019 at 03:56:19PM +0200, Noralf Trønnes wrote: >> Den 18.06.2019 15.13, skrev Laurent Pinchart: >>> The recommended way to specify GEM object functions is to provide a >>> drm_gem_object_funcs structure instance and set the GEM object to point >>> to it. The drm_cma_gem_create_object_default_funcs() function provided >>> by the GEM CMA helper does so when creating the GEM object, simplifying >>> the driver implementation. Switch to it, and remove the then unneeded >>> GEM-related opertions from rcar_du_driver. >> >> s/opertions/operations/ > > Oops, will fix. > >>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> --- >>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- >>> 1 file changed, 1 insertion(+), 7 deletions(-) >>> >>> Daniel, is this what you had in mind ? >>> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>> index 3e5e835ea2b6..4cbb82009931 100644 >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>> @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); >>> static struct drm_driver rcar_du_driver = { >>> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME >>> | DRIVER_ATOMIC, >>> - .gem_free_object_unlocked = drm_gem_cma_free_object, >>> - .gem_vm_ops = &drm_gem_cma_vm_ops, >>> + .gem_create_object = drm_cma_gem_create_object_default_funcs, >>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, >>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, >>> - .gem_prime_import = drm_gem_prime_import, >>> - .gem_prime_export = drm_gem_prime_export, >>> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, >>> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, >>> - .gem_prime_vmap = drm_gem_cma_prime_vmap, >>> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, >>> .gem_prime_mmap = drm_gem_cma_prime_mmap, >> >> If you want to pick up yet another recommendation, you can use >> drm_gem_prime_mmap here. > > I compared the two call stacks and they appear similar, even if > drm_gem_prime_mmap() leads to a more convoluted code flow. For my > information, what's the advantage in using it ? It's part of Daniels quest to remove the drm_driver gem callbacks. AFAIU drm_gem_prime_mmap() is a stop gap on the way to remove drm_driver.gem_prime_mmap. I saw it documented in his recent series: [03/59] drm/prime: Update docs https://patchwork.freedesktop.org/patch/310608/ +/** + * drm_gem_dmabuf_mmap - dma_buf mmap implementation for GEM + * @dma_buf: buffer to be mapped + * @vma: virtual address range + * + * Provides memory mapping for the buffer. This can be used as the + * &dma_buf_ops.mmap callback. It just forwards to &drm_driver.gem_prime_mmap, + * which should be set to drm_gem_prime_mmap(). + * + * FIXME: There's really no point to this wrapper, drivers which need anything + * else but drm_gem_prime_mmap can roll their own &dma_buf_ops.mmap callback. + * + * Returns 0 on success or a negative error code on failure. + */ Noralf. > >> Either way: >> >> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> >> >>> .dumb_create = rcar_du_dumb_create, >>> .fops = &rcar_du_fops, > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-18 17:23 ` Noralf Trønnes @ 2019-06-19 7:00 ` Noralf Trønnes 2019-06-19 7:42 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Noralf Trønnes @ 2019-06-19 7:00 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-renesas-soc, Daniel Vetter, Laurent Pinchart, Kieran Bingham, dri-devel Den 18.06.2019 19.23, skrev Noralf Trønnes: > > Den 18.06.2019 18.35, skrev Laurent Pinchart: >> Hi Noralf, >> >> On Tue, Jun 18, 2019 at 03:56:19PM +0200, Noralf Trønnes wrote: >>> Den 18.06.2019 15.13, skrev Laurent Pinchart: >>>> The recommended way to specify GEM object functions is to provide a >>>> drm_gem_object_funcs structure instance and set the GEM object to point >>>> to it. The drm_cma_gem_create_object_default_funcs() function provided >>>> by the GEM CMA helper does so when creating the GEM object, simplifying >>>> the driver implementation. Switch to it, and remove the then unneeded >>>> GEM-related opertions from rcar_du_driver. >>> >>> s/opertions/operations/ >> >> Oops, will fix. >> >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> --- >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- >>>> 1 file changed, 1 insertion(+), 7 deletions(-) >>>> >>>> Daniel, is this what you had in mind ? >>>> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>>> index 3e5e835ea2b6..4cbb82009931 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >>>> @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); >>>> static struct drm_driver rcar_du_driver = { >>>> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME >>>> | DRIVER_ATOMIC, >>>> - .gem_free_object_unlocked = drm_gem_cma_free_object, >>>> - .gem_vm_ops = &drm_gem_cma_vm_ops, >>>> + .gem_create_object = drm_cma_gem_create_object_default_funcs, >>>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, >>>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, >>>> - .gem_prime_import = drm_gem_prime_import, >>>> - .gem_prime_export = drm_gem_prime_export, >>>> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, >>>> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, >>>> - .gem_prime_vmap = drm_gem_cma_prime_vmap, >>>> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, >>>> .gem_prime_mmap = drm_gem_cma_prime_mmap, >>> >>> If you want to pick up yet another recommendation, you can use >>> drm_gem_prime_mmap here. >> >> I compared the two call stacks and they appear similar, even if >> drm_gem_prime_mmap() leads to a more convoluted code flow. For my >> information, what's the advantage in using it ? > > It's part of Daniels quest to remove the drm_driver gem callbacks. AFAIU > drm_gem_prime_mmap() is a stop gap on the way to remove > drm_driver.gem_prime_mmap. I saw it documented in his recent series: > [03/59] drm/prime: Update docs > https://patchwork.freedesktop.org/patch/310608/ > > +/** > + * drm_gem_dmabuf_mmap - dma_buf mmap implementation for GEM > + * @dma_buf: buffer to be mapped > + * @vma: virtual address range > + * > + * Provides memory mapping for the buffer. This can be used as the > + * &dma_buf_ops.mmap callback. It just forwards to > &drm_driver.gem_prime_mmap, > + * which should be set to drm_gem_prime_mmap(). > + * > + * FIXME: There's really no point to this wrapper, drivers which need > anything > + * else but drm_gem_prime_mmap can roll their own &dma_buf_ops.mmap > callback. It hit me that maybe it would have been better to use drm_gem_prime_mmap() as the default instead of having everyone switch to it: int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma) { struct drm_gem_object *obj = dma_buf->priv; struct drm_device *dev = obj->dev; if (!dev->driver->gem_prime_mmap) - return -ENOSYS; + return drm_gem_prime_mmap(obj, vma); return dev->driver->gem_prime_mmap(obj, vma); } Noralf. > + * > + * Returns 0 on success or a negative error code on failure. > + */ > > Noralf. > >> >>> Either way: >>> >>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> >>> >>>> .dumb_create = rcar_du_dumb_create, >>>> .fops = &rcar_du_fops, >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions 2019-06-19 7:00 ` Noralf Trønnes @ 2019-06-19 7:42 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2019-06-19 7:42 UTC (permalink / raw) To: Noralf Trønnes Cc: open list:DRM DRIVERS FOR RENESAS, Laurent Pinchart, Laurent Pinchart, dri-devel, Kieran Bingham On Wed, Jun 19, 2019 at 9:00 AM Noralf Trønnes <noralf@tronnes.org> wrote: > > > > Den 18.06.2019 19.23, skrev Noralf Trønnes: > > > > Den 18.06.2019 18.35, skrev Laurent Pinchart: > >> Hi Noralf, > >> > >> On Tue, Jun 18, 2019 at 03:56:19PM +0200, Noralf Trønnes wrote: > >>> Den 18.06.2019 15.13, skrev Laurent Pinchart: > >>>> The recommended way to specify GEM object functions is to provide a > >>>> drm_gem_object_funcs structure instance and set the GEM object to point > >>>> to it. The drm_cma_gem_create_object_default_funcs() function provided > >>>> by the GEM CMA helper does so when creating the GEM object, simplifying > >>>> the driver implementation. Switch to it, and remove the then unneeded > >>>> GEM-related opertions from rcar_du_driver. > >>> > >>> s/opertions/operations/ > >> > >> Oops, will fix. > >> > >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >>>> --- > >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 8 +------- > >>>> 1 file changed, 1 insertion(+), 7 deletions(-) > >>>> > >>>> Daniel, is this what you had in mind ? > >>>> > >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >>>> index 3e5e835ea2b6..4cbb82009931 100644 > >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >>>> @@ -445,16 +445,10 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops); > >>>> static struct drm_driver rcar_du_driver = { > >>>> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME > >>>> | DRIVER_ATOMIC, > >>>> - .gem_free_object_unlocked = drm_gem_cma_free_object, > >>>> - .gem_vm_ops = &drm_gem_cma_vm_ops, > >>>> + .gem_create_object = drm_cma_gem_create_object_default_funcs, > >>>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > >>>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > >>>> - .gem_prime_import = drm_gem_prime_import, > >>>> - .gem_prime_export = drm_gem_prime_export, > >>>> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, > >>>> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, > >>>> - .gem_prime_vmap = drm_gem_cma_prime_vmap, > >>>> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap, > >>>> .gem_prime_mmap = drm_gem_cma_prime_mmap, > >>> > >>> If you want to pick up yet another recommendation, you can use > >>> drm_gem_prime_mmap here. > >> > >> I compared the two call stacks and they appear similar, even if > >> drm_gem_prime_mmap() leads to a more convoluted code flow. For my > >> information, what's the advantage in using it ? > > > > It's part of Daniels quest to remove the drm_driver gem callbacks. AFAIU > > drm_gem_prime_mmap() is a stop gap on the way to remove > > drm_driver.gem_prime_mmap. I saw it documented in his recent series: > > [03/59] drm/prime: Update docs > > https://patchwork.freedesktop.org/patch/310608/ > > > > +/** > > + * drm_gem_dmabuf_mmap - dma_buf mmap implementation for GEM > > + * @dma_buf: buffer to be mapped > > + * @vma: virtual address range > > + * > > + * Provides memory mapping for the buffer. This can be used as the > > + * &dma_buf_ops.mmap callback. It just forwards to > > &drm_driver.gem_prime_mmap, > > + * which should be set to drm_gem_prime_mmap(). > > + * > > + * FIXME: There's really no point to this wrapper, drivers which need > > anything > > + * else but drm_gem_prime_mmap can roll their own &dma_buf_ops.mmap > > callback. > > It hit me that maybe it would have been better to use > drm_gem_prime_mmap() as the default instead of having everyone switch to it: > > int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct > *vma) > { > struct drm_gem_object *obj = dma_buf->priv; > struct drm_device *dev = obj->dev; > > if (!dev->driver->gem_prime_mmap) > - return -ENOSYS; > + return drm_gem_prime_mmap(obj, vma); > > return dev->driver->gem_prime_mmap(obj, vma); > } Crossed my mind to just do that, but it's not that easy. Right now the presence of gem_prime_mmap indicates whether mmap on the dma-buf is supported. Even for a gem driver this might not be the case, because only dump gem mmappings are supposed to be coherent. Others might need explicit cash flushing operations, which we don't set up by default (we can't). For those cases you need to implement dma_buf_ops->begin/end_cpu_access. We need to audit all the drivers, and for those which really don't implement mmap yet, provide a no_mmap dma_buf_ops->mmap implentation that just does a return -ENOSYS. Once we have that, plus also switched over everyone else, we can change drm_gem_dmabuf_mmap to unconditionally call drm_gem_prime_mmap and drop the hook. But without the full audit we can't do that, we might enable dma-buf mmap somewhere where it doesn't work. -Daniel > > Noralf. > > > + * > > + * Returns 0 on success or a negative error code on failure. > > + */ > > > > Noralf. > > > >> > >>> Either way: > >>> > >>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > >>> > >>>> .dumb_create = rcar_du_dumb_create, > >>>> .fops = &rcar_du_fops, > >> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-19 7:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-18 13:13 [PATCH] drm: rcar-du: Replace drm_driver GEM ops with GEM object functions Laurent Pinchart 2019-06-18 13:21 ` Daniel Vetter 2019-06-18 13:26 ` Laurent Pinchart 2019-06-18 13:57 ` Daniel Vetter 2019-06-18 13:56 ` Noralf Trønnes 2019-06-18 16:35 ` Laurent Pinchart 2019-06-18 17:23 ` Noralf Trønnes 2019-06-19 7:00 ` Noralf Trønnes 2019-06-19 7:42 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).