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

[Emil Velikov]

On 26 July 2017 at 19:41, Noralf Trønnes <noralf at tronnes.org> wrote:
>
> 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.
>
I was not concerned about calling the function twice, but that it's
not called at all ...
Which doesn't seems to be the case - virtually all the .dumb_create
callbacks effectively end up in __drm_gem_cma_create() which itself
calls drm_gem_create_mmap_offset().

I should have looked deeper into the rabbit hole :-)

One thing that I forgot to mention:
Depending on the tree you're working on, please grep through staging
as well - the vbox drm driver got merged somewhat recently.

>> 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.
>
Agreed - keeping that cleanup separate might be the wise thing to do.
Pardon if I came too impatient.

About a list - initial grep showed cirrus, bochs, qxl.
Now, after having a more extensive look - cirrus, qxl, ast, nouveau,
exynos, hibmc, mgag200, bochs, virtio.

On the "maybe" list - msm and omap have extra locking (is it still
needed?) while armada has an import_attach restriction (where nobody
else seems to bother?).

The remaining drivers seem a bit more complicated.

HTH
Emil