| Message ID | 20220131210552.482606-1-daniel.vetter@ffwll.ch |
|---|---|
| Headers | show |
| Series | some fbcon patches, mostly locking | expand |
On 1/31/22 22:05, Daniel Vetter wrote: > This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated > scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > option. you have two trivial copy-n-paste errors in this patch which still prevent the console acceleration. > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index 2ff90061c7f3..39dc18a5de86 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -1125,13 +1125,15 @@ static void fbcon_init(struct vc_data *vc, int init) > > ops->graphics = 0; > > - /* > - * No more hw acceleration for fbcon. > - * > - * FIXME: Garbage collect all the now dead code after sufficient time > - * has passed. > - */ > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION should be: #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > + if ((info->flags & FBINFO_HWACCEL_COPYAREA) && > + !(info->flags & FBINFO_HWACCEL_DISABLED)) > + p->scrollmode = SCROLL_MOVE; > + else /* default to something safe */ > + p->scrollmode = SCROLL_REDRAW; > +#else > p->scrollmode = SCROLL_REDRAW; > +#endif > > /* > * ++guenther: console.c:vc_allocate() relies on initializing > @@ -1971,15 +1973,49 @@ static void updatescrollmode(struct fbcon_display *p, > { > struct fbcon_ops *ops = info->fbcon_par; > int fh = vc->vc_font.height; > + int cap = info->flags; > + u16 t = 0; > + int ypan = FBCON_SWAP(ops->rotate, info->fix.ypanstep, > + info->fix.xpanstep); > + int ywrap = FBCON_SWAP(ops->rotate, info->fix.ywrapstep, t); > int yres = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); > int vyres = FBCON_SWAP(ops->rotate, info->var.yres_virtual, > info->var.xres_virtual); > + int good_pan = (cap & FBINFO_HWACCEL_YPAN) && > + divides(ypan, vc->vc_font.height) && vyres > yres; > + int good_wrap = (cap & FBINFO_HWACCEL_YWRAP) && > + divides(ywrap, vc->vc_font.height) && > + divides(vc->vc_font.height, vyres) && > + divides(vc->vc_font.height, yres); > + int reading_fast = cap & FBINFO_READS_FAST; > + int fast_copyarea = (cap & FBINFO_HWACCEL_COPYAREA) && > + !(cap & FBINFO_HWACCEL_DISABLED); > + int fast_imageblit = (cap & FBINFO_HWACCEL_IMAGEBLIT) && > + !(cap & FBINFO_HWACCEL_DISABLED); > > p->vrows = vyres/fh; > if (yres > (fh * (vc->vc_rows + 1))) > p->vrows -= (yres - (fh * vc->vc_rows)) / fh; > if ((yres % fh) && (vyres % fh < yres % fh)) > p->vrows--; > + > + if (good_wrap || good_pan) { > + if (reading_fast || fast_copyarea) > + p->scrollmode = good_wrap ? > + SCROLL_WRAP_MOVE : SCROLL_PAN_MOVE; > + else > + p->scrollmode = good_wrap ? SCROLL_REDRAW : > + SCROLL_PAN_REDRAW; > + } else { > + if (reading_fast || (fast_copyarea && !fast_imageblit)) > + p->scrollmode = SCROLL_MOVE; > + else > + p->scrollmode = SCROLL_REDRAW; > + } > + > +#ifndef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION same here... it needs to be: #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > + p->scrollmode = SCROLL_REDRAW; > +#endif > } > > #define PITCH(w) (((w) + 7) >> 3) > still reviewing the other patches... Helge
On 2/1/22 11:16, Helge Deller wrote: > On 1/31/22 22:05, Daniel Vetter wrote: >> This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated >> scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION >> option. > > you have two trivial copy-n-paste errors in this patch which still prevent the > console acceleration. > >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c >> index 2ff90061c7f3..39dc18a5de86 100644 >> --- a/drivers/video/fbdev/core/fbcon.c >> +++ b/drivers/video/fbdev/core/fbcon.c >> @@ -1125,13 +1125,15 @@ static void fbcon_init(struct vc_data *vc, int init) >> >> ops->graphics = 0; >> >> - /* >> - * No more hw acceleration for fbcon. >> - * >> - * FIXME: Garbage collect all the now dead code after sufficient time >> - * has passed. >> - */ >> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > > should be: > #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > > >> + if ((info->flags & FBINFO_HWACCEL_COPYAREA) && >> + !(info->flags & FBINFO_HWACCEL_DISABLED)) >> + p->scrollmode = SCROLL_MOVE; >> + else /* default to something safe */ >> + p->scrollmode = SCROLL_REDRAW; >> +#else >> p->scrollmode = SCROLL_REDRAW; >> +#endif >> >> /* >> * ++guenther: console.c:vc_allocate() relies on initializing >> @@ -1971,15 +1973,49 @@ static void updatescrollmode(struct fbcon_display *p, >> { >> struct fbcon_ops *ops = info->fbcon_par; >> int fh = vc->vc_font.height; >> + int cap = info->flags; >> + u16 t = 0; >> + int ypan = FBCON_SWAP(ops->rotate, info->fix.ypanstep, >> + info->fix.xpanstep); >> + int ywrap = FBCON_SWAP(ops->rotate, info->fix.ywrapstep, t); >> int yres = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); >> int vyres = FBCON_SWAP(ops->rotate, info->var.yres_virtual, >> info->var.xres_virtual); >> + int good_pan = (cap & FBINFO_HWACCEL_YPAN) && >> + divides(ypan, vc->vc_font.height) && vyres > yres; >> + int good_wrap = (cap & FBINFO_HWACCEL_YWRAP) && >> + divides(ywrap, vc->vc_font.height) && >> + divides(vc->vc_font.height, vyres) && >> + divides(vc->vc_font.height, yres); >> + int reading_fast = cap & FBINFO_READS_FAST; >> + int fast_copyarea = (cap & FBINFO_HWACCEL_COPYAREA) && >> + !(cap & FBINFO_HWACCEL_DISABLED); >> + int fast_imageblit = (cap & FBINFO_HWACCEL_IMAGEBLIT) && >> + !(cap & FBINFO_HWACCEL_DISABLED); >> >> p->vrows = vyres/fh; >> if (yres > (fh * (vc->vc_rows + 1))) >> p->vrows -= (yres - (fh * vc->vc_rows)) / fh; >> if ((yres % fh) && (vyres % fh < yres % fh)) >> p->vrows--; >> + >> + if (good_wrap || good_pan) { >> + if (reading_fast || fast_copyarea) >> + p->scrollmode = good_wrap ? >> + SCROLL_WRAP_MOVE : SCROLL_PAN_MOVE; >> + else >> + p->scrollmode = good_wrap ? SCROLL_REDRAW : >> + SCROLL_PAN_REDRAW; >> + } else { >> + if (reading_fast || (fast_copyarea && !fast_imageblit)) >> + p->scrollmode = SCROLL_MOVE; >> + else >> + p->scrollmode = SCROLL_REDRAW; >> + } >> + >> +#ifndef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > > same here... it needs to be: > #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION actually: #ifndef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > > >> + p->scrollmode = SCROLL_REDRAW; >> +#endif >> } >> >> #define PITCH(w) (((w) + 7) >> 3) >> > > still reviewing the other patches... > > Helge >
On 1/31/22 22:05, Daniel Vetter wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. Yes, agreed. Acked-by: Helge Deller <deller@gmx.de> Since the code is used by drm and existing fbdev drivers, please just make sure to not break fbdev... Thanks! Helge > > Cc: Dave Airlie <airlied@gmail.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Linux Fbdev development list <linux-fbdev@vger.kernel.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: DRI Development <dri-devel@lists.freedesktop.org> > Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Sven Schnelle <svens@stackframe.org> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > MAINTAINERS | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ea3e6c914384..49809eaa3096 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7573,6 +7573,12 @@ S: Maintained > W: http://floatingpoint.sourceforge.net/emulator/index.html > F: arch/x86/math-emu/ > > +FRAMEBUFFER CORE > +M: Daniel Vetter <daniel@ffwll.ch> > +F: drivers/video/fbdev/core/ > +S: Odd Fixes > +T: git git://anongit.freedesktop.org/drm/drm-misc > + > FRAMEBUFFER LAYER > M: Helge Deller <deller@gmx.de> > L: linux-fbdev@vger.kernel.org >
On Tue, Feb 1, 2022 at 11:16 AM Helge Deller <deller@gmx.de> wrote: > > On 1/31/22 22:05, Daniel Vetter wrote: > > This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated > > scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > > option. > > you have two trivial copy-n-paste errors in this patch which still prevent the > console acceleration. Duh :-( But before we dig into details I think the big picture would be better. I honestly don't like the #ifdef pile here that much. I wonder whether your approach, also with GETVX/YRES adjusted somehow, wouldn't look cleaner? Like I said in the cover letter I got mostly distracted with fbcon locking last week, not really with this one here at all, so maybe going with your 4 (or 2 if we squash them like I did here) patches is neater? Cheers, Daniel > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 2ff90061c7f3..39dc18a5de86 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -1125,13 +1125,15 @@ static void fbcon_init(struct vc_data *vc, int init) > > > > ops->graphics = 0; > > > > - /* > > - * No more hw acceleration for fbcon. > > - * > > - * FIXME: Garbage collect all the now dead code after sufficient time > > - * has passed. > > - */ > > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > > should be: > #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > > > > + if ((info->flags & FBINFO_HWACCEL_COPYAREA) && > > + !(info->flags & FBINFO_HWACCEL_DISABLED)) > > + p->scrollmode = SCROLL_MOVE; > > + else /* default to something safe */ > > + p->scrollmode = SCROLL_REDRAW; > > +#else > > p->scrollmode = SCROLL_REDRAW; > > +#endif > > > > /* > > * ++guenther: console.c:vc_allocate() relies on initializing > > @@ -1971,15 +1973,49 @@ static void updatescrollmode(struct fbcon_display *p, > > { > > struct fbcon_ops *ops = info->fbcon_par; > > int fh = vc->vc_font.height; > > + int cap = info->flags; > > + u16 t = 0; > > + int ypan = FBCON_SWAP(ops->rotate, info->fix.ypanstep, > > + info->fix.xpanstep); > > + int ywrap = FBCON_SWAP(ops->rotate, info->fix.ywrapstep, t); > > int yres = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); > > int vyres = FBCON_SWAP(ops->rotate, info->var.yres_virtual, > > info->var.xres_virtual); > > + int good_pan = (cap & FBINFO_HWACCEL_YPAN) && > > + divides(ypan, vc->vc_font.height) && vyres > yres; > > + int good_wrap = (cap & FBINFO_HWACCEL_YWRAP) && > > + divides(ywrap, vc->vc_font.height) && > > + divides(vc->vc_font.height, vyres) && > > + divides(vc->vc_font.height, yres); > > + int reading_fast = cap & FBINFO_READS_FAST; > > + int fast_copyarea = (cap & FBINFO_HWACCEL_COPYAREA) && > > + !(cap & FBINFO_HWACCEL_DISABLED); > > + int fast_imageblit = (cap & FBINFO_HWACCEL_IMAGEBLIT) && > > + !(cap & FBINFO_HWACCEL_DISABLED); > > > > p->vrows = vyres/fh; > > if (yres > (fh * (vc->vc_rows + 1))) > > p->vrows -= (yres - (fh * vc->vc_rows)) / fh; > > if ((yres % fh) && (vyres % fh < yres % fh)) > > p->vrows--; > > + > > + if (good_wrap || good_pan) { > > + if (reading_fast || fast_copyarea) > > + p->scrollmode = good_wrap ? > > + SCROLL_WRAP_MOVE : SCROLL_PAN_MOVE; > > + else > > + p->scrollmode = good_wrap ? SCROLL_REDRAW : > > + SCROLL_PAN_REDRAW; > > + } else { > > + if (reading_fast || (fast_copyarea && !fast_imageblit)) > > + p->scrollmode = SCROLL_MOVE; > > + else > > + p->scrollmode = SCROLL_REDRAW; > > + } > > + > > +#ifndef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > > same here... it needs to be: > #ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > > > > + p->scrollmode = SCROLL_REDRAW; > > +#endif > > } > > > > #define PITCH(w) (((w) + 7) >> 3) > > > > still reviewing the other patches... > > Helge
On 2/1/22 11:36, Daniel Vetter wrote: > On Tue, Feb 1, 2022 at 11:16 AM Helge Deller <deller@gmx.de> wrote: >> >> On 1/31/22 22:05, Daniel Vetter wrote: >>> This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated >>> scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION >>> option. >> >> you have two trivial copy-n-paste errors in this patch which still prevent the >> console acceleration. > > Duh :-( > > But before we dig into details I think the big picture would be > better. I honestly don't like the #ifdef pile here that much. Me neither. The ifdefs give a better separation, but prevents that the compiler checks the various paths when building. > I wonder whether your approach, also with GETVX/YRES adjusted > somehow, wouldn't look cleaner? I think so. You wouldn't even need to touch GETVX/YRES because the compiler will optimize/reduce it from #define GETVYRES(s,i) ({ \ (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \ (i)->var.yres : (i)->var.yres_virtual; }) to just become: #define GETVYRES(s,i) ((i)->var.yres) > Like I said in the cover letter I got mostly distracted with fbcon > locking last week, not really with this one here at all, so maybe > going with your 4 (or 2 if we squash them like I did here) patches is > neater? The benefit of my patch series was, that it could be easily backported first, and then cleaned up afterwards. Even a small additional backport patch to disable the fbcon acceleration for DRM in the old releases would be easy. But I'm not insisting on backporting the patches, if we find good way forward. So, either with the 4 (or 2) patches would be OK for me (or even your approach). Helge
On Tue, Feb 1, 2022 at 12:01 PM Helge Deller <deller@gmx.de> wrote: > On 2/1/22 11:36, Daniel Vetter wrote: > > On Tue, Feb 1, 2022 at 11:16 AM Helge Deller <deller@gmx.de> wrote: > >> > >> On 1/31/22 22:05, Daniel Vetter wrote: > >>> This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated > >>> scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > >>> option. > >> > >> you have two trivial copy-n-paste errors in this patch which still prevent the > >> console acceleration. > > > > Duh :-( > > > > But before we dig into details I think the big picture would be > > better. I honestly don't like the #ifdef pile here that much. > > Me neither. > The ifdefs give a better separation, but prevents that the compiler > checks the various paths when building. > > > I wonder whether your approach, also with GETVX/YRES adjusted > > somehow, wouldn't look cleaner? > I think so. > You wouldn't even need to touch GETVX/YRES because the compiler > will optimize/reduce it from > > #define GETVYRES(s,i) ({ \ > (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \ > (i)->var.yres : (i)->var.yres_virtual; }) > > to just become: > > #define GETVYRES(s,i) ((i)->var.yres) Yeah, but you need to roll out your helper to all the callsites. But since you #ifdef out info->scrollmode we should catch them all I guess. > > Like I said in the cover letter I got mostly distracted with fbcon > > locking last week, not really with this one here at all, so maybe > > going with your 4 (or 2 if we squash them like I did here) patches is > > neater? > > The benefit of my patch series was, that it could be easily backported first, > and then cleaned up afterwards. Even a small additional backport patch to disable > the fbcon acceleration for DRM in the old releases would be easy. > But I'm not insisting on backporting the patches, if we find good way forward. > > So, either with the 4 (or 2) patches would be OK for me (or even your approach). The idea behind the squash was that it's then impossible to backport without the Kconfig, and so we'll only enable this code when people intentionally want it. Maybe I'm too paranoid? Anyway, you feel like finishing off your approach? Or should I send out v2 of this with the issues fixed you spotted? Like I said either is fine with me. -Daniel
On 1/31/22 22:05, Daniel Vetter wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. > > Cc: Dave Airlie <airlied@gmail.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Linux Fbdev development list <linux-fbdev@vger.kernel.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: DRI Development <dri-devel@lists.freedesktop.org> > Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Sven Schnelle <svens@stackframe.org> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards,
On Mon, 31 Jan 2022, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. > > Cc: Dave Airlie <airlied@gmail.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Linux Fbdev development list <linux-fbdev@vger.kernel.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: DRI Development <dri-devel@lists.freedesktop.org> > Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Sven Schnelle <svens@stackframe.org> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > MAINTAINERS | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ea3e6c914384..49809eaa3096 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7573,6 +7573,12 @@ S: Maintained > W: http://floatingpoint.sourceforge.net/emulator/index.html > F: arch/x86/math-emu/ > > +FRAMEBUFFER CORE > +M: Daniel Vetter <daniel@ffwll.ch> > +F: drivers/video/fbdev/core/ > +S: Odd Fixes > +T: git git://anongit.freedesktop.org/drm/drm-misc > + > FRAMEBUFFER LAYER > M: Helge Deller <deller@gmx.de> > L: linux-fbdev@vger.kernel.org
On 2/1/22 14:45, Daniel Vetter wrote: > On Tue, Feb 1, 2022 at 12:01 PM Helge Deller <deller@gmx.de> wrote: >> On 2/1/22 11:36, Daniel Vetter wrote: >>> On Tue, Feb 1, 2022 at 11:16 AM Helge Deller <deller@gmx.de> wrote: >>>> >>>> On 1/31/22 22:05, Daniel Vetter wrote: >>>>> This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated >>>>> scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION >>>>> option. >>>> >>>> you have two trivial copy-n-paste errors in this patch which still prevent the >>>> console acceleration. >>> >>> Duh :-( >>> >>> But before we dig into details I think the big picture would be >>> better. I honestly don't like the #ifdef pile here that much. >> >> Me neither. >> The ifdefs give a better separation, but prevents that the compiler >> checks the various paths when building. >> >>> I wonder whether your approach, also with GETVX/YRES adjusted >>> somehow, wouldn't look cleaner? >> I think so. >> You wouldn't even need to touch GETVX/YRES because the compiler >> will optimize/reduce it from >> >> #define GETVYRES(s,i) ({ \ >> (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \ >> (i)->var.yres : (i)->var.yres_virtual; }) >> >> to just become: >> >> #define GETVYRES(s,i) ((i)->var.yres) > > Yeah, but you need to roll out your helper to all the callsites. But > since you #ifdef out info->scrollmode we should catch them all I > guess. Right. That was the only reason why I ifdef'ed it out. Technically we don't need that ifdef. >>> Like I said in the cover letter I got mostly distracted with fbcon >>> locking last week, not really with this one here at all, so maybe >>> going with your 4 (or 2 if we squash them like I did here) patches is >>> neater? >> >> The benefit of my patch series was, that it could be easily backported first, >> and then cleaned up afterwards. Even a small additional backport patch to disable >> the fbcon acceleration for DRM in the old releases would be easy. >> But I'm not insisting on backporting the patches, if we find good way forward. >> >> So, either with the 4 (or 2) patches would be OK for me (or even your approach). > > The idea behind the squash was that it's then impossible to backport > without the Kconfig, Yes, my proposal was to simply revert the 2 patches and immediatly send the Kconfig patch to disable it again. > and so we'll only enable this code when people > intentionally want it. Maybe I'm too paranoid? I think you are too paranoid :-) If all patches incl. the Kconfig patch are backported then people shouldn't do it wrong. > Anyway, you feel like finishing off your approach? Or should I send > out v2 of this with the issues fixed you spotted? Like I said either > is fine with me. Ok, then let me try to finish my approach until tomorrow, and then you check if you can and want to add your locking and other patches on top of it. In the end I leave the decision which approach to take to you. Ok? Helge
On Mon, Jan 31, 2022 at 10:06 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Feb 1, 2022 at 3:52 PM Helge Deller <deller@gmx.de> wrote: > > On 2/1/22 14:45, Daniel Vetter wrote: > > On Tue, Feb 1, 2022 at 12:01 PM Helge Deller <deller@gmx.de> wrote: > >> On 2/1/22 11:36, Daniel Vetter wrote: > >>> On Tue, Feb 1, 2022 at 11:16 AM Helge Deller <deller@gmx.de> wrote: > >>>> > >>>> On 1/31/22 22:05, Daniel Vetter wrote: > >>>>> This functionally undoes 39aead8373b3 ("fbcon: Disable accelerated > >>>>> scrolling"), but behind the FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION > >>>>> option. > >>>> > >>>> you have two trivial copy-n-paste errors in this patch which still prevent the > >>>> console acceleration. > >>> > >>> Duh :-( > >>> > >>> But before we dig into details I think the big picture would be > >>> better. I honestly don't like the #ifdef pile here that much. > >> > >> Me neither. > >> The ifdefs give a better separation, but prevents that the compiler > >> checks the various paths when building. > >> > >>> I wonder whether your approach, also with GETVX/YRES adjusted > >>> somehow, wouldn't look cleaner? > >> I think so. > >> You wouldn't even need to touch GETVX/YRES because the compiler > >> will optimize/reduce it from > >> > >> #define GETVYRES(s,i) ({ \ > >> (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \ > >> (i)->var.yres : (i)->var.yres_virtual; }) > >> > >> to just become: > >> > >> #define GETVYRES(s,i) ((i)->var.yres) > > > > Yeah, but you need to roll out your helper to all the callsites. But > > since you #ifdef out info->scrollmode we should catch them all I > > guess. > > Right. That was the only reason why I ifdef'ed it out. > Technically we don't need that ifdef. > > >>> Like I said in the cover letter I got mostly distracted with fbcon > >>> locking last week, not really with this one here at all, so maybe > >>> going with your 4 (or 2 if we squash them like I did here) patches is > >>> neater? > >> > >> The benefit of my patch series was, that it could be easily backported first, > >> and then cleaned up afterwards. Even a small additional backport patch to disable > >> the fbcon acceleration for DRM in the old releases would be easy. > >> But I'm not insisting on backporting the patches, if we find good way forward. > >> > >> So, either with the 4 (or 2) patches would be OK for me (or even your approach). > > > > The idea behind the squash was that it's then impossible to backport > > without the Kconfig, > > Yes, my proposal was to simply revert the 2 patches and immediatly send > the Kconfig patch to disable it again. > > > and so we'll only enable this code when people > > intentionally want it. Maybe I'm too paranoid? > > I think you are too paranoid :-) > If all patches incl. the Kconfig patch are backported then people shouldn't > do it wrong. > > > Anyway, you feel like finishing off your approach? Or should I send > > out v2 of this with the issues fixed you spotted? Like I said either > > is fine with me. > > Ok, then let me try to finish my approach until tomorrow, and then you > check if you can and want to add your locking and other patches on top of it. > In the end I leave the decision which approach to take to you. > Ok? Sounds good, and yeah rough idea is that the maintainers + revert + Kconfig should go in for rc3 or rc4 if we hit another bump, and the locking stuff then in for -next (since it needs a backmerge and is defo tricky stuff). Cheers, Daniel
On Tue, 1 Feb 2022 at 07:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin Acked-by: Dave Airlie <airlied@redhat.com>
Hi all, This took way longer than I hoped, but well I got lost in head-scratching locking problems. Anyway ended up typing a pile of fbcon patches. Rough overview: - MAINTAINER entry for fbdev core in drm-misc, with the usual group maintainering - The reverts, but with a compile time option. I looked into combining this with the RFC from Helge, but it looked like worse than either approach, so I've kept mine. I think merging either is fine, no personal stake here on the bikeshed color. - The real distraction, aka a bunch of cleanups and mostly locking work for fbcon. I managed to ditch at least one locking FIXME from fbcon.c. The bad news that I spent a bunch more time pondering about the bigger locking issues in fbcon.c, and I think as-is they're unfixable. We need the threaded printk support to land first, so that we're not adding lock_fb_info() to all printk() contexts. Because that just cannot work. That also means we'll likely have another fbcon regression to face, in the form of worse oops printing ability. And that one I don't think we can fix with clever application of #ifdef, because changing locking rules at compile time in fundamental ways is just not a very good idea. Anyway, testing, review, feedback and all that very much welcome, as usual. Cheers, Daniel Daniel Vetter (21): MAINTAINERS: Add entry for fbdev core fbcon: Resurrect fbcon accelerated scrolling code fbcon: Restore fbcon scrolling acceleration fbcon: delete a few unneeded forward decl fbcon: Introduce wrapper for console->fb_info lookup fbcon: delete delayed loading code fbdev/sysfs: Fix locking fbcon: Use delayed work for cursor fbcon: Replace FBCON_FLAGS_INIT with a boolean fb: Delete fb_info->queue fbcon: Extract fbcon_open/release helpers fbcon: Ditch error handling for con2fb_release_oldinfo fbcon: move more common code into fb_open() fbcon: use lock_fb_info in fbcon_open/release fbcon: Consistently protect deferred_takeover with console_lock() fbcon: Move console_lock for register/unlink/unregister fbcon: Move more code into fbcon_release fbcon: untangle fbcon_exit fbcon: Maintain a private array of fb_info Revert "fbdev: Prevent probing generic drivers if a FB is already registered" fbdev: Make registered_fb[] private to fbmem.c Documentation/gpu/todo.rst | 13 +- MAINTAINERS | 6 + drivers/video/console/Kconfig | 11 + drivers/video/fbdev/core/bitblit.c | 16 + drivers/video/fbdev/core/fbcon.c | 1072 +++++++++++++++++------ drivers/video/fbdev/core/fbcon.h | 67 +- drivers/video/fbdev/core/fbcon_ccw.c | 28 +- drivers/video/fbdev/core/fbcon_cw.c | 28 +- drivers/video/fbdev/core/fbcon_rotate.h | 21 + drivers/video/fbdev/core/fbcon_ud.c | 37 +- drivers/video/fbdev/core/fbmem.c | 35 +- drivers/video/fbdev/core/fbsysfs.c | 2 + drivers/video/fbdev/core/tileblit.c | 16 + drivers/video/fbdev/efifb.c | 11 - drivers/video/fbdev/simplefb.c | 11 - drivers/video/fbdev/skeletonfb.c | 12 +- include/linux/fb.h | 10 +- 17 files changed, 1017 insertions(+), 379 deletions(-)