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

[Noralf Trønnes]

Den 27.07.2017 02.13, skrev 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.

All of those except exynos gets their offset from 
ttm_buffer_object->vma_node
and not drm_gem_object.vma_node. Is it the same vma_node?

I'll add exynos in the second round, I missed that one.

Thanks for the scrutiny, it's appreciated.

Noralf.

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