On Wed, Jun 19, 2019 at 9:00 AM Noralf Trønnes <noralf@xxxxxxxxxxx> 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@xxxxxxxx>
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxx>
> >>>
> >>>> .dumb_create = rcar_du_dumb_create,
> >>>> .fops = &rcar_du_fops,
> >>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
