| Message ID | 20230515094033.2133-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm/fbdev: Remove DRM's helpers for fbdev I/O | expand |
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@suse.de> > Cc: Russell King <linux@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. Sam > .fb_destroy = armada_fbdev_fb_destroy, > }; > > -- > 2.40.1
Hi Am 15.05.23 um 19:55 schrieb Sam Ravnborg: > 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@suse.de> >> Cc: Russell King <linux@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? fb_io_read() and fb_io_write() are the default implementations. They are called when no callback has been set. All the other fbdev drivers leave them out, so I kept this pattern for the DRM side as well. Best regards Thomas > > 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. > > Sam > >> .fb_destroy = armada_fbdev_fb_destroy, >> }; >> >> -- >> 2.40.1
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@suse.de> >>> Cc: Russell King <linux@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@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@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 >