fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line, option

Commit Message

Peter Rosin Jan. 7, 2019, 8:26 a.m. UTC

On 2019-01-06 10:33, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <[e-mail]> wrote:
>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
>> extra logos are not considered when centering the first logo vertically.
>>
>> Signed-off-by: Peter Rosin <[e-mail]>
> 
>> --- a/drivers/video/logo/Kconfig
>> +++ b/drivers/video/logo/Kconfig
>> @@ -10,6 +10,15 @@ menuconfig LOGO
>>
>>  if LOGO
>>
>> +config FB_LOGO_CENTER
>> +       bool "Center the logo"
>> +       depends on FB=y
>> +       help
>> +         When this option is selected, the bootup logo is centered both
>> +         horizontally and vertically. If more than one logo is displayed
>> +         due to multiple CPUs, the collected line of logos is centered
>> +         as a whole.
>> +
> 
> Isn't a kernel command line option more suitable to configure the position
> of the logo?

That didn't occur to me previously, but it does make sense now that you
mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
can avoid possible regressions for folks who might start to rely on
CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.

Cheers,
Peter

From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
From: Peter Rosin <[e-mail]>
Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
Cc: Jonathan Corbet <[e-mail]>
Cc: Peter Rosin <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
To: [e-mail]
Date: Mon, 7 Jan 2019 08:35:26 +0100
Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
 option

A command line option is much more flexible than a config option and
the supporting code is small. Gets rid of #ifdefs in the code too...

Suggested-by: Geert Uytterhoeven <[e-mail]>
Signed-off-by: Peter Rosin <[e-mail]>
---
 Documentation/fb/fbcon.txt       |  6 ++++++
 drivers/video/fbdev/core/fbcon.c |  5 +++++
 drivers/video/fbdev/core/fbmem.c | 19 ++++++++++---------
 drivers/video/logo/Kconfig       |  9 ---------
 include/linux/fb.h               |  1 +
 5 files changed, 22 insertions(+), 18 deletions(-)

Comments

Geert Uytterhoeven Jan. 7, 2019, 8:40 a.m. UTC | #1

Hi Peter,

On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <[e-mail]> wrote:
> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
> > On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <[e-mail]> wrote:
> >> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
> >> extra logos are not considered when centering the first logo vertically.
> >>
> >> Signed-off-by: Peter Rosin <[e-mail]>
> >
> >> --- a/drivers/video/logo/Kconfig
> >> +++ b/drivers/video/logo/Kconfig
> >> @@ -10,6 +10,15 @@ menuconfig LOGO
> >>
> >>  if LOGO
> >>
> >> +config FB_LOGO_CENTER
> >> +       bool "Center the logo"
> >> +       depends on FB=y
> >> +       help
> >> +         When this option is selected, the bootup logo is centered both
> >> +         horizontally and vertically. If more than one logo is displayed
> >> +         due to multiple CPUs, the collected line of logos is centered
> >> +         as a whole.
> >> +
> >
> > Isn't a kernel command line option more suitable to configure the position
> > of the logo?
>
> That didn't occur to me previously, but it does make sense now that you
> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
> can avoid possible regressions for folks who might start to rely on
> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
>
> Cheers,
> Peter
>
> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
> From: Peter Rosin <[e-mail]>
> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Peter Rosin <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> To: [e-mail]
> Date: Mon, 7 Jan 2019 08:35:26 +0100
> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
>  option
>
> A command line option is much more flexible than a config option and
> the supporting code is small. Gets rid of #ifdefs in the code too...
>
> Suggested-by: Geert Uytterhoeven <[e-mail]>
> Signed-off-by: Peter Rosin <[e-mail]>

Thanks for your patch!

> --- a/Documentation/fb/fbcon.txt
> +++ b/Documentation/fb/fbcon.txt
> @@ -163,6 +163,12 @@ C. Boot options
>         be preserved until there actually is some text is output to the console.
>         This option causes fbcon to bind immediately to the fbdev device.
>
> +7. fbcon=center-logo
> +
> +       When this option is selected, the bootup logo is centered both
> +       horizontally and vertically. If more than one logo is displayed due to
> +       multiple CPUs, the collected line of logos is centered as a whole.
> +

What about making this more generic, than an option specific for centering?
Else people want fbcon=left-logo or fbcon=right-logo soon?

    fbcon=logo-pos:<pos>

With <pos> being "center", "left", "top", "right", "bottom", and combinations
like "top,center"? Or is that complicating stuff too much?

Gr{oetje,eeting}s,

                        Geert

Peter Rosin Jan. 7, 2019, 8:59 a.m. UTC | #2

On 2019-01-07 09:40, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <[e-mail]> wrote:
>> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <[e-mail]> wrote:
>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
>>>> extra logos are not considered when centering the first logo vertically.
>>>>
>>>> Signed-off-by: Peter Rosin <[e-mail]>
>>>
>>>> --- a/drivers/video/logo/Kconfig
>>>> +++ b/drivers/video/logo/Kconfig
>>>> @@ -10,6 +10,15 @@ menuconfig LOGO
>>>>
>>>>  if LOGO
>>>>
>>>> +config FB_LOGO_CENTER
>>>> +       bool "Center the logo"
>>>> +       depends on FB=y
>>>> +       help
>>>> +         When this option is selected, the bootup logo is centered both
>>>> +         horizontally and vertically. If more than one logo is displayed
>>>> +         due to multiple CPUs, the collected line of logos is centered
>>>> +         as a whole.
>>>> +
>>>
>>> Isn't a kernel command line option more suitable to configure the position
>>> of the logo?
>>
>> That didn't occur to me previously, but it does make sense now that you
>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
>> can avoid possible regressions for folks who might start to rely on
>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
>>
>> Cheers,
>> Peter
>>
>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
>> From: Peter Rosin <[e-mail]>
>> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
>> Cc: Jonathan Corbet <[e-mail]>
>> Cc: Peter Rosin <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> To: [e-mail]
>> Date: Mon, 7 Jan 2019 08:35:26 +0100
>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
>>  option
>>
>> A command line option is much more flexible than a config option and
>> the supporting code is small. Gets rid of #ifdefs in the code too...
>>
>> Suggested-by: Geert Uytterhoeven <[e-mail]>
>> Signed-off-by: Peter Rosin <[e-mail]>
> 
> Thanks for your patch!
> 
>> --- a/Documentation/fb/fbcon.txt
>> +++ b/Documentation/fb/fbcon.txt
>> @@ -163,6 +163,12 @@ C. Boot options
>>         be preserved until there actually is some text is output to the console.
>>         This option causes fbcon to bind immediately to the fbdev device.
>>
>> +7. fbcon=center-logo
>> +
>> +       When this option is selected, the bootup logo is centered both
>> +       horizontally and vertically. If more than one logo is displayed due to
>> +       multiple CPUs, the collected line of logos is centered as a whole.
>> +
> 
> What about making this more generic, than an option specific for centering?
> Else people want fbcon=left-logo or fbcon=right-logo soon?
> 
>     fbcon=logo-pos:<pos>
> 
> With <pos> being "center", "left", "top", "right", "bottom", and combinations
> like "top,center"? Or is that complicating stuff too much?

I'm not too keen on implementing all that when I have zero use for it. I can
however rename the trigger for the existing fb_center_logo bool to

	fbcon=logo-pos:center

and add a check for "top,left" to reset the bool to false. Then the other
variants can be added later by whomever and when needed.

Is that good enough?

Cheers,
Peter

Peter Rosin Jan. 7, 2019, 9:02 a.m. UTC | #3

On 2019-01-07 09:59, Peter Rosin wrote:
> On 2019-01-07 09:40, Geert Uytterhoeven wrote:
>> Hi Peter,
>>
>> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <[e-mail]> wrote:
>>> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
>>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <[e-mail]> wrote:
>>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
>>>>> extra logos are not considered when centering the first logo vertically.
>>>>>
>>>>> Signed-off-by: Peter Rosin <[e-mail]>
>>>>
>>>>> --- a/drivers/video/logo/Kconfig
>>>>> +++ b/drivers/video/logo/Kconfig
>>>>> @@ -10,6 +10,15 @@ menuconfig LOGO
>>>>>
>>>>>  if LOGO
>>>>>
>>>>> +config FB_LOGO_CENTER
>>>>> +       bool "Center the logo"
>>>>> +       depends on FB=y
>>>>> +       help
>>>>> +         When this option is selected, the bootup logo is centered both
>>>>> +         horizontally and vertically. If more than one logo is displayed
>>>>> +         due to multiple CPUs, the collected line of logos is centered
>>>>> +         as a whole.
>>>>> +
>>>>
>>>> Isn't a kernel command line option more suitable to configure the position
>>>> of the logo?
>>>
>>> That didn't occur to me previously, but it does make sense now that you
>>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
>>> can avoid possible regressions for folks who might start to rely on
>>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
>>>
>>> Cheers,
>>> Peter
>>>
>>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
>>> From: Peter Rosin <[e-mail]>
>>> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
>>> Cc: Jonathan Corbet <[e-mail]>
>>> Cc: Peter Rosin <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> To: [e-mail]
>>> Date: Mon, 7 Jan 2019 08:35:26 +0100
>>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
>>>  option
>>>
>>> A command line option is much more flexible than a config option and
>>> the supporting code is small. Gets rid of #ifdefs in the code too...
>>>
>>> Suggested-by: Geert Uytterhoeven <[e-mail]>
>>> Signed-off-by: Peter Rosin <[e-mail]>
>>
>> Thanks for your patch!
>>
>>> --- a/Documentation/fb/fbcon.txt
>>> +++ b/Documentation/fb/fbcon.txt
>>> @@ -163,6 +163,12 @@ C. Boot options
>>>         be preserved until there actually is some text is output to the console.
>>>         This option causes fbcon to bind immediately to the fbdev device.
>>>
>>> +7. fbcon=center-logo
>>> +
>>> +       When this option is selected, the bootup logo is centered both
>>> +       horizontally and vertically. If more than one logo is displayed due to
>>> +       multiple CPUs, the collected line of logos is centered as a whole.
>>> +
>>
>> What about making this more generic, than an option specific for centering?
>> Else people want fbcon=left-logo or fbcon=right-logo soon?
>>
>>     fbcon=logo-pos:<pos>
>>
>> With <pos> being "center", "left", "top", "right", "bottom", and combinations
>> like "top,center"? Or is that complicating stuff too much?
> 
> I'm not too keen on implementing all that when I have zero use for it. I can
> however rename the trigger for the existing fb_center_logo bool to
> 
> 	fbcon=logo-pos:center
> 
> and add a check for "top,left" to reset the bool to false. Then the other
> variants can be added later by whomever and when needed.
> 
> Is that good enough?

Ouch, I just realized that using a comma is a no-go, as that is already
separating fbcon suboptions, so I suggest top-left instead.

Cheers,
Peter

Geert Uytterhoeven Jan. 7, 2019, 9:11 a.m. UTC | #4

Hi Peter,

On Mon, Jan 7, 2019 at 10:03 AM Peter Rosin <[e-mail]> wrote:
> On 2019-01-07 09:59, Peter Rosin wrote:
> > On 2019-01-07 09:40, Geert Uytterhoeven wrote:
> >> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <[e-mail]> wrote:
> >>> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
> >>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <[e-mail]> wrote:
> >>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
> >>>>> extra logos are not considered when centering the first logo vertically.
> >>>>>
> >>>>> Signed-off-by: Peter Rosin <[e-mail]>
> >>>>
> >>>>> --- a/drivers/video/logo/Kconfig
> >>>>> +++ b/drivers/video/logo/Kconfig
> >>>>> @@ -10,6 +10,15 @@ menuconfig LOGO
> >>>>>
> >>>>>  if LOGO
> >>>>>
> >>>>> +config FB_LOGO_CENTER
> >>>>> +       bool "Center the logo"
> >>>>> +       depends on FB=y
> >>>>> +       help
> >>>>> +         When this option is selected, the bootup logo is centered both
> >>>>> +         horizontally and vertically. If more than one logo is displayed
> >>>>> +         due to multiple CPUs, the collected line of logos is centered
> >>>>> +         as a whole.
> >>>>> +
> >>>>
> >>>> Isn't a kernel command line option more suitable to configure the position
> >>>> of the logo?
> >>>
> >>> That didn't occur to me previously, but it does make sense now that you
> >>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
> >>> can avoid possible regressions for folks who might start to rely on
> >>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
> >>>
> >>> Cheers,
> >>> Peter
> >>>
> >>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
> >>> From: Peter Rosin <[e-mail]>
> >>> Cc: Bartlomiej Zolnierkiewicz <[e-mail]>
> >>> Cc: Jonathan Corbet <[e-mail]>
> >>> Cc: Peter Rosin <[e-mail]>
> >>> Cc: [e-mail]
> >>> Cc: [e-mail]
> >>> Cc: [e-mail]
> >>> To: [e-mail]
> >>> Date: Mon, 7 Jan 2019 08:35:26 +0100
> >>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
> >>>  option
> >>>
> >>> A command line option is much more flexible than a config option and
> >>> the supporting code is small. Gets rid of #ifdefs in the code too...
> >>>
> >>> Suggested-by: Geert Uytterhoeven <[e-mail]>
> >>> Signed-off-by: Peter Rosin <[e-mail]>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/Documentation/fb/fbcon.txt
> >>> +++ b/Documentation/fb/fbcon.txt
> >>> @@ -163,6 +163,12 @@ C. Boot options
> >>>         be preserved until there actually is some text is output to the console.
> >>>         This option causes fbcon to bind immediately to the fbdev device.
> >>>
> >>> +7. fbcon=center-logo
> >>> +
> >>> +       When this option is selected, the bootup logo is centered both
> >>> +       horizontally and vertically. If more than one logo is displayed due to
> >>> +       multiple CPUs, the collected line of logos is centered as a whole.
> >>> +
> >>
> >> What about making this more generic, than an option specific for centering?
> >> Else people want fbcon=left-logo or fbcon=right-logo soon?
> >>
> >>     fbcon=logo-pos:<pos>
> >>
> >> With <pos> being "center", "left", "top", "right", "bottom", and combinations
> >> like "top,center"? Or is that complicating stuff too much?
> >
> > I'm not too keen on implementing all that when I have zero use for it. I can
> > however rename the trigger for the existing fb_center_logo bool to
> >
> >       fbcon=logo-pos:center
> >
> > and add a check for "top,left" to reset the bool to false. Then the other
> > variants can be added later by whomever and when needed.
> >
> > Is that good enough?
>
> Ouch, I just realized that using a comma is a no-go, as that is already
> separating fbcon suboptions, so I suggest top-left instead.

Sure, sounds fine to me.  I just want to avoid having a parameter system
that complicates future extension.
I think you can drop the check for top-left, as the bool defaults to false.

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/Documentation/fb/fbcon.txt b/Documentation/fb/fbcon.txt
index 62af30511a95..fb5fa0a8d553 100644
--- a/Documentation/fb/fbcon.txt
+++ b/Documentation/fb/fbcon.txt
@@ -163,6 +163,12 @@  C. Boot options
 	be preserved until there actually is some text is output to the console.
 	This option causes fbcon to bind immediately to the fbdev device.
 
+7. fbcon=center-logo
+
+	When this option is selected, the bootup logo is centered both
+	horizontally and vertically. If more than one logo is displayed due to
+	multiple CPUs, the collected line of logos is centered as a whole.
+
 C. Attaching, Detaching and Unloading
 
 Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 8976190b6c1f..552cfee63d76 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -510,6 +510,11 @@  static int __init fb_console_setup(char *this_opt)
 			continue;
 		}
 #endif
+
+		if (!strcmp(options, "center-logo")) {
+			fb_center_logo = true;
+			continue;
+		}
 	}
 	return 1;
 }
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 558ed2ed3124..cb43a2258c51 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -53,6 +53,9 @@  EXPORT_SYMBOL(registered_fb);
 int num_registered_fb __read_mostly;
 EXPORT_SYMBOL(num_registered_fb);
 
+bool fb_center_logo __read_mostly;
+EXPORT_SYMBOL(fb_center_logo);
+
 static struct fb_info *get_fb_info(unsigned int idx)
 {
 	struct fb_info *fb_info;
@@ -506,8 +509,7 @@  static int fb_show_logo_line(struct fb_info *info, int rotate,
 		fb_set_logo(info, logo, logo_new, fb_logo.depth);
 	}
 
-#ifdef CONFIG_FB_LOGO_CENTER
-	{
+	if (fb_center_logo) {
 		int xres = info->var.xres;
 		int yres = info->var.yres;
 
@@ -520,11 +522,11 @@  static int fb_show_logo_line(struct fb_info *info, int rotate,
 			--n;
 		image.dx = (xres - n * (logo->width + 8) - 8) / 2;
 		image.dy = y ?: (yres - logo->height) / 2;
+	} else {
+		image.dx = 0;
+		image.dy = y;
 	}
-#else
-	image.dx = 0;
-	image.dy = y;
-#endif
+
 	image.width = logo->width;
 	image.height = logo->height;
 
@@ -684,9 +686,8 @@  int fb_prepare_logo(struct fb_info *info, int rotate)
  	}
 
 	height = fb_logo.logo->height;
-#ifdef CONFIG_FB_LOGO_CENTER
-	height += (yres - fb_logo.logo->height) / 2;
-#endif
+	if (fb_center_logo)
+		height += (yres - fb_logo.logo->height) / 2;
 
 	return fb_prepare_extra_logos(info, height, yres);
 }
diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig
index 1e972c4e88b1..d1f6196c8b9a 100644
--- a/drivers/video/logo/Kconfig
+++ b/drivers/video/logo/Kconfig
@@ -10,15 +10,6 @@  menuconfig LOGO
 
 if LOGO
 
-config FB_LOGO_CENTER
-	bool "Center the logo"
-	depends on FB=y
-	help
-	  When this option is selected, the bootup logo is centered both
-	  horizontally and vertically. If more than one logo is displayed
-	  due to multiple CPUs, the collected line of logos is centered
-	  as a whole.
-
 config FB_LOGO_EXTRA
 	bool
 	depends on FB=y
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 7cdd31a69719..f52ef0ad6781 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -653,6 +653,7 @@  extern int fb_new_modelist(struct fb_info *info);
 
 extern struct fb_info *registered_fb[FB_MAX];
 extern int num_registered_fb;
+extern bool fb_center_logo;
 extern struct class *fb_class;
 
 #define for_each_registered_fb(i)		\