[PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers

[Thomas Zimmermann]

Hi

Am 08.03.22 um 20:29 schrieb Javier Martinez Canillas:
> On 3/3/22 21:58, Thomas Zimmermann wrote:
>> Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The
>> created buffer object returned by this function implements deferred
>> I/O for its mmap operation.
>>
>> Add this feature to a number of drivers that use GEM SHMEM helpers
>> as shadow planes over their regular video memory. The new macro
>> DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM
>> functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation
>> on these drivers will now mmap the GEM SHMEM pages directly with
>> deferred I/O without an intermediate shadow buffer.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
> 
> [snip]
> 
>> @@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>   	.vm_ops = &drm_gem_shmem_vm_ops,
>>   };
>>   
>> +static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {
>> +	.free = drm_gem_shmem_object_free,
>> +	.print_info = drm_gem_shmem_object_print_info,
>> +	.pin = drm_gem_shmem_object_pin,
>> +	.unpin = drm_gem_shmem_object_unpin,
>> +	.get_sg_table = drm_gem_shmem_object_get_sg_table,
>> +	.vmap = drm_gem_shmem_object_vmap,
>> +	.vunmap = drm_gem_shmem_object_vunmap,
>> +	.mmap = drm_gem_shmem_object_mmap_fbdev,
>> +	.vm_ops = &drm_gem_shmem_vm_ops_fbdev,
> 
> The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but
> .mmap and .vm_ops callbacks. Maybe adding a macro to declare these two
> struct drm_gem_object_funcs could make easier to maintain it long term ?

I see you point, but I'm undecided about this. Such macros also add some 
amount of obfuscation. I'm not sure if it's worth it for an internal 
constant. And since the fbdev buffer is never exported, we could remove 
some of the callbacks. AFAICT we never call pin, unpin or get_sg_table.

Best regards
Thomas

> 
>> +};
>> +
>>   static struct drm_gem_shmem_object *
>> -__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
>> +__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private, bool fbdev)
>>   {
>>   	struct drm_gem_shmem_object *shmem;
>>   	struct drm_gem_object *obj;
>> @@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
>>   		obj = &shmem->base;
>>   	}
>>   
>> -	if (!obj->funcs)
>> -		obj->funcs = &drm_gem_shmem_funcs;
>> +	if (!obj->funcs) {
>> +		if (fbdev)
> 
> Same question than in patch #6, maybe
> 
>                  if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
> 
> [snip]
> 
>> + */
>> +int drm_gem_shmem_dumb_create_fbdev(struct drm_file *file, struct drm_device *dev,
>> +				    struct drm_mode_create_dumb *args)
>> +{
>> +#if defined(CONFIG_DRM_FBDEV_EMULATION)
>> +	return __drm_gem_shmem_dumb_create(file, dev, true, args);
>> +#else
>> +	return -ENOSYS;
>> +#endif
>> +}
> 
> I believe the correct errno code here should be -ENOTSUPP.
> 
> [snip]
> 
>> +	/* We don't use vmf->pgoff since that has the fake offset */
>> +	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
> 
> I see this (vmf->address - vma->vm_start) calculation in many places of
> this series. Maybe we could add a macro to calculate the offset instead ?
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220309/a177ff9b/attachment-0001.sig>