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: 4100 bytes --]

Hi

Am 08.03.22 um 18:51 schrieb Javier Martinez Canillas:
> On 3/3/22 21:58, Thomas Zimmermann wrote:
>> Provide struct drm_driver.dumb_create_fbdev, a callback hook for
>> fbdev dumb buffers. Wire up fbdev and client helpers to use the new
>> interface, if present.
>>
>> This acknowledges the fact that fbdev buffers are different. The most
>> significant difference to regular GEM BOs is in mmap semantics. Fbdev
>> userspace treats the pages as video memory, which makes it impossible
>> to ever move the mmap'ed buffer. Hence, drivers ussually have to pin
>> the BO permanently or install an intermediate shadow buffer for mmap.
>>
>> So far, fbdev memory came from dumb buffers and DRM drivers had no
>> means of detecting this without reimplementing a good part of the fbdev
>> code. With the new callback, drivers can perma-pin fbdev buffer objects
>> if needed.
>>
>> Several drivers also require damage handling, which fbdev implements
>> with its deferred I/O helpers. The new callback allows a driver's memory
>> manager to set up a suitable mmap.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_client.c        | 14 +++++++----
>>   drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>>   drivers/gpu/drm/drm_dumb_buffers.c  | 36 +++++++++++++++++++++++++----
>>   drivers/gpu/drm/drm_fb_helper.c     | 26 +++++++++++++++++----
>>   include/drm/drm_client.h            |  3 ++-
>>   include/drm/drm_drv.h               | 17 ++++++++++++++
>>   6 files changed, 84 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index af3b7395bf69..c964064651d5 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -247,7 +247,8 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>>   }
>>   
>>   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.

Makes sense.

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

Makes sense, I think.

Fbdev emulation is the only user of this code. And there won't be other 
helpers like that. Any other client would use the regular dumb-buffer 
functions.

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

Ok.

Best regards
Thomas

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