drm: rcar-du: Replace drm_driver GEM ops with GEM object functions

Commit Message

Laurent Pinchart June 18, 2019, 1:13 p.m. UTC

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 <[e-mail]>
Signed-off-by: Laurent Pinchart <[e-mail]>
---
 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 ?

Comments

Daniel Vetter June 18, 2019, 1:21 p.m. UTC | #1

On Tue, Jun 18, 2019 at 3:13 PM Laurent Pinchart <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
>  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 <[e-mail]>


>
> 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
[phone] - http://blog.ffwll.ch

Laurent Pinchart June 18, 2019, 1:26 p.m. UTC | #2

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 <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Laurent Pinchart <[e-mail]>
> > ---
> >  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 <[e-mail]>
> 
> > 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,

Noralf Trønnes June 18, 2019, 1:56 p.m. UTC | #3

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 <[e-mail]>
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
>  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 <[e-mail]>

>  	.dumb_create		= rcar_du_dumb_create,
>  	.fops			= &rcar_du_fops,
>

Daniel Vetter June 18, 2019, 1:57 p.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Laurent Pinchart <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> > 
> > > 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

Laurent Pinchart June 18, 2019, 4:35 p.m. UTC | #5

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 <[e-mail]>
> > Signed-off-by: Laurent Pinchart <[e-mail]>
> > ---
> >  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 <[e-mail]>
> 
> >  	.dumb_create		= rcar_du_dumb_create,
> >  	.fops			= &rcar_du_fops,

Noralf Trønnes June 18, 2019, 5:23 p.m. UTC | #6

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 <[e-mail]>
>>> Signed-off-by: Laurent Pinchart <[e-mail]>
>>> ---
>>>  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 <[e-mail]>
>>
>>>  	.dumb_create		= rcar_du_dumb_create,
>>>  	.fops			= &rcar_du_fops,
>

Noralf Trønnes June 19, 2019, 7 a.m. UTC | #7

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 <[e-mail]>
>>>> Signed-off-by: Laurent Pinchart <[e-mail]>
>>>> ---
>>>>  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 <[e-mail]>
>>>
>>>>  	.dumb_create		= rcar_du_dumb_create,
>>>>  	.fops			= &rcar_du_fops,
>>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Daniel Vetter June 19, 2019, 7:42 a.m. UTC | #8

On Wed, Jun 19, 2019 at 9:00 AM Noralf Trønnes <[e-mail]> 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 <[e-mail]>
> >>>> Signed-off-by: Laurent Pinchart <[e-mail]>
> >>>> ---
> >>>>  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 <[e-mail]>
> >>>
> >>>>    .dumb_create            = rcar_du_dumb_create,
> >>>>    .fops                   = &rcar_du_fops,
> >>
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >

Patch

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,