Re: [PATCH 1/8] fbdev/smscufx: Use fb_ops helpers for deferred I/O

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Javier Martinez Canillas <javierm@redhat.com>,
	deller@gmx.de, daniel@ffwll.ch, sam@ravnborg.org
Cc: Steve Glendinning <steve.glendinning@shawell.net>,
	linux-fbdev@vger.kernel.org, linux-staging@lists.linux.dev,
	linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-input@vger.kernel.org
Subject: Re: [PATCH 1/8] fbdev/smscufx: Use fb_ops helpers for deferred I/O
Date: Mon, 4 Sep 2023 16:39:47 +0200	[thread overview]
Message-ID: <b9b985e7-4f60-7c59-3121-b26b07b13b03@suse.de> (raw)
In-Reply-To: <877cp66qmp.fsf@minerva.mail-host-address-is-not-set>

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

Hi Javier

Am 04.09.23 um 14:59 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Generate callback functions for struct fb_ops with the fbdev macro
>> FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to
>> the generated functions with fbdev initializer macros.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Steve Glendinning <steve.glendinning@shawell.net>
>> ---
> 
> The patch looks good to me, but I've a question below.
> 
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>>   drivers/video/fbdev/smscufx.c | 85 +++++++++--------------------------
>>   1 file changed, 22 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
> 
> [...]
> 
>>   static const struct fb_ops ufx_ops = {
>>   	.owner = THIS_MODULE,
>> -	.fb_read = fb_sys_read,
>> -	.fb_write = ufx_ops_write,
>> +	__FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops),
>>   	.fb_setcolreg = ufx_ops_setcolreg,
>> -	.fb_fillrect = ufx_ops_fillrect,
>> -	.fb_copyarea = ufx_ops_copyarea,
>> -	.fb_imageblit = ufx_ops_imageblit,
>> +	__FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops),
>>   	.fb_mmap = ufx_ops_mmap,
> 
> There are no generated functions for .fb_mmap, I wonder what's the value
> of __FB_DEFAULT_DEFERRED_OPS_MMAP() ? Maybe just removing that macro and
> setting .fb_mmap = fb_deferred_io_mmap instead if there's no custom mmap
> handler would be easier to read ?

At least two drivers could use __FB_DEFAULT_DEFERRED_OPS_MMAP: 
picolcd-fb and hyperv_fb. At some point, we might want to set/clear 
fb_mmap depending on some Kconfig value. Having 
__FB_DEFAULT_DEFERRED_OPS_MMAP might be helpful then.

> 
> Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but
> not taking a __prefix argument since that is not used anyways ?

The driver optionally provides mmap without deferred I/O, hence the mmap 
function. That makes no sense, as these writes to the buffer would never 
make it to the device memory. But I didn't want to remove the code 
either. So I just left the existing function as-is. Usually, the 
deferred-I/O mmap is called immediately. [1]

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.5.1/source/drivers/video/fbdev/smscufx.c#L784

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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