[PATCH 01/59] drm/todo: Improve drm_gem_object funcs todo

[Noralf Trønnes]

Den 17.06.2019 23.20, skrev Daniel Vetter:
> On Mon, Jun 17, 2019 at 06:54:04PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 17.06.2019 18.29, skrev Daniel Vetter:
>>> On Mon, Jun 17, 2019 at 05:47:50PM +0200, Noralf Trønnes wrote:
>>>>
>>>>
>>>> Den 14.06.2019 22.35, skrev Daniel Vetter:
>>>>> We're kinda going in the wrong direction. Spotted while typing better
>>>>> gem/prime docs.
>>>>>
>>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>>>> Cc: Rob Herring <robh at kernel.org>
>>>>> Cc: Noralf Trønnes <noralf at tronnes.org>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>>>> ---
>>>>>  Documentation/gpu/todo.rst | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>>>>> index b4a76c2703e5..23583f0e3755 100644
>>>>> --- a/Documentation/gpu/todo.rst
>>>>> +++ b/Documentation/gpu/todo.rst
>>>>> @@ -228,6 +228,10 @@ struct drm_gem_object_funcs
>>>>>  GEM objects can now have a function table instead of having the callbacks on the
>>>>>  DRM driver struct. This is now the preferred way and drivers can be moved over.
>>>>>  
>>>>> +Unfortunately some of the recently added GEM helpers are going in the wrong
>>>>> +direction by adding OPS macros that use the old, deprecated hooks. See
>>>>> +DRM_GEM_CMA_VMAP_DRIVER_OPS, DRM_GEM_SHMEM_DRIVER_OPS, and DRM_GEM_VRAM_DRIVER_PRIME.
>>>>> +
>>>>
>>>> Both DRM_GEM_CMA_VMAP_DRIVER_OPS and DRM_GEM_SHMEM_DRIVER_OPS use the
>>>> GEM vtable. Or am I missing something here?
>>>
>>> gem vtable I mean drm_gem_object_funcs. Which these macros definitely
>>> aren't useful for.
>>
>> #define DRM_GEM_CMA_VMAP_DRIVER_OPS \
>> 	.gem_create_object	= drm_cma_gem_create_object_default_funcs, \
>> 	.dumb_create		= drm_gem_cma_dumb_create, \
>> 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
>> 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
>> 	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \
>> 	.gem_prime_mmap		= drm_gem_prime_mmap
>>
>> __drm_gem_cma_create() calls ->gem_create_object.
>>
>> drm_cma_gem_create_object_default_funcs() sets:
>> 	cma_obj->base.funcs = &drm_cma_gem_default_funcs;
>>
>> static const struct drm_gem_object_funcs drm_cma_gem_default_funcs = {
>> 	.free = drm_gem_cma_free_object,
>> 	.print_info = drm_gem_cma_print_info,
>> 	.get_sg_table = drm_gem_cma_prime_get_sg_table,
>> 	.vmap = drm_gem_cma_prime_vmap,
>> 	.vm_ops = &drm_gem_cma_vm_ops,
>> };
>>
>> The GEM SHMEM helper was made after drm_gem_object_funcs came about so
>> it sets the default vtable in drm_gem_shmem_create():
>> 		obj->funcs = &drm_gem_shmem_funcs;
>>
>> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>> 	.free = drm_gem_shmem_free_object,
>> 	.print_info = drm_gem_shmem_print_info,
>> 	.pin = drm_gem_shmem_pin,
>> 	.unpin = drm_gem_shmem_unpin,
>> 	.get_sg_table = drm_gem_shmem_get_sg_table,
>> 	.vmap = drm_gem_shmem_vmap,
>> 	.vunmap = drm_gem_shmem_vunmap,
>> 	.vm_ops = &drm_gem_shmem_vm_ops,
>> };
>>
>> #define DRM_GEM_SHMEM_DRIVER_OPS \
>> 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
>> 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
>> 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \
>> 	.gem_prime_mmap		= drm_gem_prime_mmap, \
>> 	.dumb_create		= drm_gem_shmem_dumb_create
>>
>> So the two driver ops macroes only set the necessary bits to enable
>> prime import/export/mmap and dumb buffer creation, leaving the rest to
>> drm_gem_object_funcs.
>> Have we deprecated any of these hooks?
> 
> Uh I was blind :-/ Unfortunately I pushed that patch already, I'll follow
> up with a patch to fix it. vram helpers are not following latest best
> practices though, right?

Right.

> 
> Also I guess a lot more of the cma helper using drivers could be cut over
> to the vmap ones?

DRM_GEM_CMA_VMAP_DRIVER_OPS is for drivers that need a virtual address
also on imported buffers. Currently only tinydrm drivers require this.

Other drivers that don't change the defaults could use this:

#define DRM_GEM_CMA_DRIVER_OPS \
	.gem_create_object	= drm_cma_gem_create_object_default_funcs, \
	.dumb_create		= drm_gem_cma_dumb_create, \
	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, \
	.gem_prime_mmap		= drm_gem_prime_mmap

I see that aspeed is a good first candidate for using such a macro since
it's already open coding it.

Noralf.