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

[Noralf Trønnes]

Den 26.07.2017 14.05, skrev Emil Velikov:
> Hi Noralf,
>
>
> On 23 July 2017 at 20:16, Noralf Trønnes <noralf at tronnes.org> wrote:
>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>
>> Signed-off-by: Noralf Trønnes <noralf at 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 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.

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/cirrus_main.c#n292
>