Re: [PATCH 01/41] drm/gem: Add drm_gem_dumb_map_offset()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Noralf,

On Wednesday 26 Jul 2017 20:41:53 Noralf Trønnes wrote:
> Den 26.07.2017 14.05, skrev Emil Velikov:
> > On 23 July 2017 at 20:16, Noralf Trønnes <noralf@tronnes.org> wrote:
> >> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> >> 
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >> 
> >>   drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++++++++++++++++++++
> >>   include/drm/drm_gem.h     |  2 ++
> >>   2 files changed, 37 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >> index 5df028a..a8d396b 100644
> >> --- a/drivers/gpu/drm/drm_gem.c
> >> +++ b/drivers/gpu/drm/drm_gem.c
> >> @@ -311,6 +311,41 @@ drm_gem_handle_delete(struct drm_file *filp, u32
> >> handle)>> 
> >>   EXPORT_SYMBOL(drm_gem_handle_delete);
> >>   
> >>   /**
> >> + * drm_gem_dumb_map_offset - return the fake mmap offset for a gem
> >> object
> >> + * @file: drm file-private structure containing the gem object
> >> + * @dev: corresponding drm_device
> >> + * @handle: gem object handle
> >> + * @offset: return location for the fake mmap offset
> >> + *
> >> + * This implements the &drm_driver.dumb_map_offset kms driver callback
> >> for
> >> + * drivers which use gem to manage their backing storage.
> >> + *
> >> + * Returns:
> >> + * 0 on success or a negative error code on failure.
> >> + */
> >> +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device
> >> *dev,
> >> +                           u32 handle, u64 *offset)
> >> +{
> >> +       struct drm_gem_object *obj;
> >> +       int ret;
> >> +
> >> +       obj = drm_gem_object_lookup(file, handle);
> >> +       if (!obj)
> >> +               return -ENOENT;
> >> +
> >> +       ret = drm_gem_create_mmap_offset(obj);
> > 
> > With later patches one goes to reuse this helper instead of
> > drm_gem_cma_dumb_map_offset().
> > At the same time, the latter does not have the
> > drm_gem_create_mmap_offset() call we see here.
> 
> You're rigth, the cma library and a couple of other drivers if I recall
> correctly, call drm_gem_create_mmap_offset() during object creation.
> Daniel was of the opinion that this should happen when mmap is called.
> drm_gem_create_mmap_offset() is idempotent as the docs call it. Calling
> it a second time is ok, since it does nothing (needs to take a lock
> though).

I came to the exact same conclusion. It might be a slightly more efficient if 
we coudl call drm_gem_create_mmap_offset() at the GEM object creation time, 
but as that's not possible for all drivers, and as the .dumb_map_offset() 
operation is not in any hot path anyway, I stopped investigating how we could 
optimize the implementation.

> I haven't removed drm_gem_create_mmap_offset() from the drivers that do
> it during object creation, trying to keep this from doing to much.
> The cma library will be changed though when I add the generic fb/gem
> helper.
> 
> > Meanwhile some drivers have their own offset function which is
> > virtually identical to the original drm_gem_cma_dumb_map_offset.
> > Yet those are left unchanged.
> > 
> > For example in cirrus [1]:
> > There the only code difference seems to be an "upcast" followed
> > immediately by a "downcast". Which should effectively be a noop.
> 
> The main reason for that is to try and keep this simple so I'm able to
> add my shmem/gem library in reasonable time :-) I didn't want to do
> changes that wasn't straight forward. But yes, the cirrus drivers looks
> quite straight forward and the functions isn't used by other parts of
> the driver. I looked at a driver (omap?) that had similar functions,
> and those where called from other parts of the driver, so I expected
> the same here I guess.

For omapdrm I believe we could drop the custom .dumb_map_offset() operation, 
as the only use for customs offsets in the driver is for tiled buffers, which 
are created by an omapdrm-specific IOCTL. However, I can't tell at the moment 
whether userspace code that create tiled buffers with the OMAP_GEM_NEW ioctl 
get the mmap offset with OMAP_GEM_INFO as they should, or if 
DRM_IOCTL_MODE_MAP_DUMB gets sometimes abused for non-dumb buffers.

> You say "some drivers", can you name them?
> Some drivers takes locks or do other stuff that made me skip them.
> 
> Noralf.
> 
> > That said, I could be loosing my marbles.
> > 
> > HTH
> > Emil
> > 
> > [1]
> > https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/cirrus/cir
> > rus_main.c#n292

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel