[Intel-gfx] [PATCH v2 02/12] drm/armada: Use regular fbdev I/O helpers

[Thomas Zimmermann]

Hi

Am 15.05.23 um 20:04 schrieb Russell King (Oracle):
> On Mon, May 15, 2023 at 07:55:44PM +0200, Sam Ravnborg wrote:
>> Hi Thomas,
>>
>> On Mon, May 15, 2023 at 11:40:23AM +0200, Thomas Zimmermann wrote:
>>> Use the regular fbdev helpers for framebuffer I/O instead of DRM's
>>> helpers. Armada does not use damage handling, so DRM's fbdev helpers
>>> are mere wrappers around the fbdev code.
>>>
>>> By using fbdev helpers directly within each DRM fbdev emulation,
>>> we can eventually remove DRM's wrapper functions entirely.
>>>
>>> v2:
>>> 	* use FB_IO_HELPERS option
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> Cc: Russell King <linux at armlinux.org.uk>
>>> ---
>>>   drivers/gpu/drm/armada/Kconfig        | 1 +
>>>   drivers/gpu/drm/armada/armada_fbdev.c | 9 ++++-----
>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig
>>> index f5c66d89ba99..5afade25e217 100644
>>> --- a/drivers/gpu/drm/armada/Kconfig
>>> +++ b/drivers/gpu/drm/armada/Kconfig
>>> @@ -3,6 +3,7 @@ config DRM_ARMADA
>>>   	tristate "DRM support for Marvell Armada SoCs"
>>>   	depends on DRM && HAVE_CLK && ARM && MMU
>>>   	select DRM_KMS_HELPER
>>> +	select FB_IO_HELPERS if DRM_FBDEV_EMULATION
>>>   	help
>>>   	  Support the "LCD" controllers found on the Marvell Armada 510
>>>   	  devices.  There are two controllers on the device, each controller
>>> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
>>> index 0a5fd1aa86eb..6c3bbaf53569 100644
>>> --- a/drivers/gpu/drm/armada/armada_fbdev.c
>>> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
>>> @@ -5,6 +5,7 @@
>>>    */
>>>   
>>>   #include <linux/errno.h>
>>> +#include <linux/fb.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   
>>> @@ -34,11 +35,9 @@ static void armada_fbdev_fb_destroy(struct fb_info *info)
>>>   static const struct fb_ops armada_fb_ops = {
>>>   	.owner		= THIS_MODULE,
>>>   	DRM_FB_HELPER_DEFAULT_OPS,
>>> -	.fb_read	= drm_fb_helper_cfb_read,
>>> -	.fb_write	= drm_fb_helper_cfb_write,
>> I had expected to see
>> .fb_read = fb_io_read,
>>
>> But maybe this only used when using damage handling?
>>
>> Likewise for drm_fb_helper_cfb_write.
>>
>> ??
>>
>>> -	.fb_fillrect	= drm_fb_helper_cfb_fillrect,
>>> -	.fb_copyarea	= drm_fb_helper_cfb_copyarea,
>>> -	.fb_imageblit	= drm_fb_helper_cfb_imageblit,
>>> +	.fb_fillrect	= cfb_fillrect,
>>> +	.fb_copyarea	= cfb_copyarea,
>>> +	.fb_imageblit	= cfb_imageblit,
>>
>> This part is as expected.
> 
> Well, to me it looks like this has gone through an entire circular set
> of revisions:
> 
> commit e8b70e4dd7b5dad7c2379de6e0851587bf86bfd6
> Author: Archit Taneja <architt at codeaurora.org>
> Date:   Wed Jul 22 14:58:04 2015 +0530
> 
>      drm/armada: Use new drm_fb_helper functions
> 
> -       .fb_fillrect    = cfb_fillrect,
> -       .fb_copyarea    = cfb_copyarea,
> -       .fb_imageblit   = cfb_imageblit,
> +       .fb_fillrect    = drm_fb_helper_cfb_fillrect,
> +       .fb_copyarea    = drm_fb_helper_cfb_copyarea,
> +       .fb_imageblit   = drm_fb_helper_cfb_imageblit,
> 
> commit 983780918c759fdbbf0bf033e701bbff75d2af23
> Author: Thomas Zimmermann <tzimmermann at suse.de>
> Date:   Thu Nov 3 16:14:40 2022 +0100
> 
>      drm/fb-helper: Perform all fbdev I/O with the same implementation
> 
> +       .fb_read        = drm_fb_helper_cfb_read,
> +       .fb_write       = drm_fb_helper_cfb_write,
> 
> and now effectively those two changes are being reverted, so we'd
> now be back to the pre-July 2015 state of affairs. As I believe
> the fbdev layer has been stable, this change merely reverts the
> driver back to what it once was.

Not quite. One long-standing problem has been that fbdev does not 
protect its public interfaces with CONFIG_FB. If fbdev had been 
disabled, DRM drivers could no longer be linked/loaded. DRM wrappers 
solved this. The issue has recently been fixed for all of DRM. DRM does 
not build it's fbdev emulation if CONFIG_FB has been disabled.

Another thing was that the original DRM wrappers might have been 
different from fbdev's I/O helpers in subtle ways. But now they are 
simple wrappers around their fbdev counterparts; plus the option of 
additional damage handling.  But such damage handling is better 
implemented by the driver itself. The two cases that require it, i915 
and fbdev-generic, are different enough that each should probably have 
it's own code.

Best regards
Thomas

> 

-- 
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)
-------------- 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/intel-gfx/attachments/20230516/0f0578b8/attachment.sig>