Re: [PATCH 6/9] drm/fb-helper: Provide callback to create fbdev dumb buffers

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2618 bytes --]

Hi Javier

Am 08.03.22 um 18:51 schrieb Javier Martinez Canillas:
[...]
>>   
>>   static struct drm_client_buffer *
>> -drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
>> +drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format,
>> +			 bool fbdev)
>>   {
>>   	const struct drm_format_info *info = drm_format_info(format);
>>   	struct drm_mode_create_dumb dumb_args = { };
>> @@ -265,7 +266,10 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>>   	dumb_args.width = width;
>>   	dumb_args.height = height;
>>   	dumb_args.bpp = info->cpp[0] * 8;
>> -	ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
>> +	if (fbdev)
> 
> Maybe if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ?
> 
>> +		ret = drm_mode_create_dumb_fbdev(dev, &dumb_args, client->file);
> 
> And drm_mode_create_dumb_fbdev() could just be made a stub if
> CONFIG_DRM_FBDEV_EMULATION isn't enabled.

While going through these commits again, I decided to change the logic 
to use a function pointer for the allocation. The fbdev emulation will 
provide an implementation that allocates the buffer object. The logic 
for picking the correct create-dumb function will be placed there.

Although it probably won't ever be needed, other clients could allocate 
buffers with other constraints, or even support acceleration instead of 
dumb buffers.

You may want to take another look at this patch in the patchset's next 
iteration.

Best regards
Thomas

> 
> I believe the only usage of the DRM client API currently is the fbdev
> emulation layer anyways? But still makes sense I think to condtionally
> compile that since drm_client.o is built in the drm.ko module and the
> drm_fb_helper.o only included if fbdev emulation has been enabled.
> 
>> +	else
>> +		ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
>>   	if (ret)
>>   		goto err_delete;
>>   
>> @@ -402,6 +406,7 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
>>    * @width: Framebuffer width
>>    * @height: Framebuffer height
>>    * @format: Buffer format
>> + * @fbdev: True if the client provides an fbdev device, or false otherwise
>>    *
> 
> An emulated fbdev device ?
> 
> Other than those small nit,
> 
> Reviewed-by: Javier Martinez Canillas <javierm@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]