Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 18, 2017 at 11:06:56PM +0200, Noralf Trønnes wrote:
> 
> Den 12.07.2017 15.45, skrev Noralf Trønnes:
> > 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 8dc1106..44ecbaa 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);
> 
> There are 3 drm_driver->dumb_map_offset implementations that
> don't call drm_gem_create_mmap_offset():
> - drm_gem_cma_dumb_map_offset()
> - exynos_drm_gem_dumb_map_offset()
> - tegra_bo_dumb_map_offset()
> 
> They do it during object creation.
> 
> exynos have this commit:
> drm/exynos: create a fake mmap offset with gem creation
> 48cf53f4343ae12ddc1c60dbe116161ecf7a2885
> 
> I looked at the discussion but didn't understand the rationale:
> https://lists.freedesktop.org/archives/dri-devel/2015-August/088541.html

This discussion indeed doesn't make sense - Inki says the patch doesn't
make sense and then still goes ahead and merges it.

> I see that it's ok to call drm_gem_create_mmap_offset() multiple times,
> so it's not really a problem for this function, but it would be nice to
> know why for my shmem gem library which is based on the cma library.

I don't think there's any reasonable reason for this. Lazy creating of
offsets is perfectly fine imo, we might as well bake that in as the
standard. And for some drivers it's required (you'll run out of mmap
space), but modern userspace should be all using mmap64.

Only thing to make sure is that the driver-specific mmap also creates the
offset when needed, when we remove it from gem_init functions.
-Daniel

> 
> Noralf.
> 
> 
> > +	if (ret)
> > +		goto out;
> > +
> > +	*offset = drm_vma_node_offset_addr(&obj->vma_node);
> > +out:
> > +	drm_gem_object_put_unlocked(obj);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
> > +
> > +/**
> >    * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
> >    * @file: drm file-private structure to remove the dumb handle from
> >    * @dev: corresponding drm_device
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 4a9d231..9c55c2a 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -302,6 +302,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> >   		bool dirty, bool accessed);
> >   struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> > +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > +			    u32 handle, u64 *offset);
> >   int drm_gem_dumb_destroy(struct drm_file *file,
> >   			 struct drm_device *dev,
> >   			 uint32_t handle);
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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