diff mbox series

[1/5] fbdev/efifb: Use stack memory for screeninfo structs

Message ID 20240827-efifb-sysfs-v1-1-c9cc3e052180@weissschuh.net (mailing list archive)
State Rejected, archived
Headers show
Series fbdev: devm_register_framebuffer() and some fixes for efifb | expand

Commit Message

Thomas Weißschuh Aug. 27, 2024, 3:25 p.m. UTC 
These variables are only used inside efifb_probe().
Afterwards they are using memory unnecessarily.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Helge Deller Aug. 28, 2024, 5:42 p.m. UTC | #1 
On 8/27/24 17:25, Thomas Weißschuh wrote:
> These variables are only used inside efifb_probe().
> Afterwards they are using memory unnecessarily.

Did you check if this change really saves some memory?
With your change, the compiler will either create a hidden
structure which it uses then, or it generates assembly
instructions to fill the struct at runtime.
Both options may not actually reduce the memory footprint...

Another option might be to mark the static struct __initdata
if you expect another card to take over before the memory is
freed at runtime. But I'm not sure if it's worth possible
implications.

Helge

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 8dd82afb3452..8bfe0ccbc67a 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -52,24 +52,6 @@ struct efifb_par {
>   	resource_size_t size;
>   };
>
> -static struct fb_var_screeninfo efifb_defined = {
> -	.activate		= FB_ACTIVATE_NOW,
> -	.height			= -1,
> -	.width			= -1,
> -	.right_margin		= 32,
> -	.upper_margin		= 16,
> -	.lower_margin		= 4,
> -	.vsync_len		= 4,
> -	.vmode			= FB_VMODE_NONINTERLACED,
> -};
> -
> -static struct fb_fix_screeninfo efifb_fix = {
> -	.id			= "EFI VGA",
> -	.type			= FB_TYPE_PACKED_PIXELS,
> -	.accel			= FB_ACCEL_NONE,
> -	.visual			= FB_VISUAL_TRUECOLOR,
> -};
> -
>   static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
>   			   unsigned blue, unsigned transp,
>   			   struct fb_info *info)
> @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
>   	char *option = NULL;
>   	efi_memory_desc_t md;
>
> +	struct fb_var_screeninfo efifb_defined = {
> +		.activate		= FB_ACTIVATE_NOW,
> +		.height			= -1,
> +		.width			= -1,
> +		.right_margin		= 32,
> +		.upper_margin		= 16,
> +		.lower_margin		= 4,
> +		.vsync_len		= 4,
> +		.vmode			= FB_VMODE_NONINTERLACED,
> +	};
> +
> +	struct fb_fix_screeninfo efifb_fix = {
> +		.id			= "EFI VGA",
> +		.type			= FB_TYPE_PACKED_PIXELS,
> +		.accel			= FB_ACCEL_NONE,
> +		.visual			= FB_VISUAL_TRUECOLOR,
> +	};
> +
>   	/*
>   	 * If we fail probing the device, the kernel might try a different
>   	 * driver. We get a copy of the attached screen_info, so that we can
>
Thomas Weißschuh Aug. 29, 2024, 7:52 a.m. UTC | #2 
On 2024-08-28 19:42:51+0000, Helge Deller wrote:
> On 8/27/24 17:25, Thomas Weißschuh wrote:
> > These variables are only used inside efifb_probe().
> > Afterwards they are using memory unnecessarily.
> 
> Did you check if this change really saves some memory?

Nope...

> With your change, the compiler will either create a hidden
> structure which it uses then, or it generates assembly
> instructions to fill the struct at runtime.
> Both options may not actually reduce the memory footprint...

Thanks for the explanation, it makes sense.

On advantage of the on-stack data would be future-proofing.
Efi efifb_probe() overrides some fields in these structs only in certain
codepaths then the globally shared data could become inconsistent.
But that's not the case today.

> Another option might be to mark the static struct __initdata
> if you expect another card to take over before the memory is
> freed at runtime. But I'm not sure if it's worth possible
> implications.

Agreed.

> Helge
> 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
> >   1 file changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 8dd82afb3452..8bfe0ccbc67a 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -52,24 +52,6 @@ struct efifb_par {
> >   	resource_size_t size;
> >   };
> > 
> > -static struct fb_var_screeninfo efifb_defined = {
> > -	.activate		= FB_ACTIVATE_NOW,
> > -	.height			= -1,
> > -	.width			= -1,
> > -	.right_margin		= 32,
> > -	.upper_margin		= 16,
> > -	.lower_margin		= 4,
> > -	.vsync_len		= 4,
> > -	.vmode			= FB_VMODE_NONINTERLACED,
> > -};
> > -
> > -static struct fb_fix_screeninfo efifb_fix = {
> > -	.id			= "EFI VGA",
> > -	.type			= FB_TYPE_PACKED_PIXELS,
> > -	.accel			= FB_ACCEL_NONE,
> > -	.visual			= FB_VISUAL_TRUECOLOR,
> > -};
> > -
> >   static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
> >   			   unsigned blue, unsigned transp,
> >   			   struct fb_info *info)
> > @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
> >   	char *option = NULL;
> >   	efi_memory_desc_t md;
> > 
> > +	struct fb_var_screeninfo efifb_defined = {
> > +		.activate		= FB_ACTIVATE_NOW,
> > +		.height			= -1,
> > +		.width			= -1,
> > +		.right_margin		= 32,
> > +		.upper_margin		= 16,
> > +		.lower_margin		= 4,
> > +		.vsync_len		= 4,
> > +		.vmode			= FB_VMODE_NONINTERLACED,
> > +	};
> > +
> > +	struct fb_fix_screeninfo efifb_fix = {
> > +		.id			= "EFI VGA",
> > +		.type			= FB_TYPE_PACKED_PIXELS,
> > +		.accel			= FB_ACCEL_NONE,
> > +		.visual			= FB_VISUAL_TRUECOLOR,
> > +	};
> > +
> >   	/*
> >   	 * If we fail probing the device, the kernel might try a different
> >   	 * driver. We get a copy of the attached screen_info, so that we can
> > 
>
diff mbox series

Patch

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 8dd82afb3452..8bfe0ccbc67a 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -52,24 +52,6 @@  struct efifb_par {
 	resource_size_t size;
 };
 
-static struct fb_var_screeninfo efifb_defined = {
-	.activate		= FB_ACTIVATE_NOW,
-	.height			= -1,
-	.width			= -1,
-	.right_margin		= 32,
-	.upper_margin		= 16,
-	.lower_margin		= 4,
-	.vsync_len		= 4,
-	.vmode			= FB_VMODE_NONINTERLACED,
-};
-
-static struct fb_fix_screeninfo efifb_fix = {
-	.id			= "EFI VGA",
-	.type			= FB_TYPE_PACKED_PIXELS,
-	.accel			= FB_ACCEL_NONE,
-	.visual			= FB_VISUAL_TRUECOLOR,
-};
-
 static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
 			   unsigned blue, unsigned transp,
 			   struct fb_info *info)
@@ -357,6 +339,24 @@  static int efifb_probe(struct platform_device *dev)
 	char *option = NULL;
 	efi_memory_desc_t md;
 
+	struct fb_var_screeninfo efifb_defined = {
+		.activate		= FB_ACTIVATE_NOW,
+		.height			= -1,
+		.width			= -1,
+		.right_margin		= 32,
+		.upper_margin		= 16,
+		.lower_margin		= 4,
+		.vsync_len		= 4,
+		.vmode			= FB_VMODE_NONINTERLACED,
+	};
+
+	struct fb_fix_screeninfo efifb_fix = {
+		.id			= "EFI VGA",
+		.type			= FB_TYPE_PACKED_PIXELS,
+		.accel			= FB_ACCEL_NONE,
+		.visual			= FB_VISUAL_TRUECOLOR,
+	};
+
 	/*
 	 * If we fail probing the device, the kernel might try a different
 	 * driver. We get a copy of the attached screen_info, so that we can