Re: [v2, 1/4] fbdev/efifb: Replace references to global screen_info by local pointer

Hi

Am 04.12.23 um 20:53 schrieb Sui Jingfeng:

Hi,


On 2023/12/4 17:27, Thomas Zimmermann wrote:

quoted

Get the global screen_info's address once and access the data via
this pointer. Limits the use of global state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Sui Jingfeng <redacted>


I have applied the whole series and do a simple test with efifb:
Yeah, still works!

$ dmesg | grep efifb

[    0.373800] efifb: probing for efifb
[    0.373816] efifb: framebuffer at 0xe0030000000, using 5120k, total 
5120k
[    0.373818] efifb: mode is 1280x1024x32, linelength=5120, pages=1
[    0.373820] efifb: scrolling: redraw
[    0.373821] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0

quoted

---
  drivers/video/fbdev/efifb.c | 113 ++++++++++++++++++------------------
  1 file changed, 58 insertions(+), 55 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index f9b4ddd592ce4..6cbb65bbe1110 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c

@@ -147,10 +147,9 @@ static bool efifb_bgrt_sanity_check(struct 

screen_info *si, u32 bmp_width)
  }
  #endif
-static void efifb_show_boot_graphics(struct fb_info *info)
+static void efifb_show_boot_graphics(struct fb_info *info, struct 
screen_info *si)

I think, we probably can add a const modifier for the function
which doesn't modify our local the screen_info instance in the future.

Good point. All our source code is traditionally sloppy with const 
correctness.

Best regards
Thomas

static void efifb_show_boot_graphics(struct fb_info *info, const struct 
screen_info *si)

Since screen_info is mostly used to provide information.

quoted

  {
      u32 bmp_width, bmp_height, bmp_pitch, dst_x, y, src_y;
-    struct screen_info *si = &screen_info;
      struct bmp_file_header *file_header;
      struct bmp_dib_header *dib_header;
      void *bgrt_image = NULL;

@@ -282,7 +281,7 @@ static const struct fb_ops efifb_ops = {

      .fb_setcolreg    = efifb_setcolreg,
  };
-static int efifb_setup(char *options)
+static int efifb_setup(struct screen_info *si, char *options)
  {
      char *this_opt;

@@ -290,16 +289,16 @@ static int efifb_setup(char *options)

          while ((this_opt = strsep(&options, ",")) != NULL) {
              if (!*this_opt) continue;
-            efifb_setup_from_dmi(&screen_info, this_opt);
+            efifb_setup_from_dmi(si, this_opt);
              if (!strncmp(this_opt, "base:", 5))
-                screen_info.lfb_base = simple_strtoul(this_opt+5, 
NULL, 0);
+                si->lfb_base = simple_strtoul(this_opt+5, NULL, 0);
              else if (!strncmp(this_opt, "stride:", 7))
-                screen_info.lfb_linelength = 
simple_strtoul(this_opt+7, NULL, 0) * 4;
+                si->lfb_linelength = simple_strtoul(this_opt+7, NULL, 
0) * 4;
              else if (!strncmp(this_opt, "height:", 7))
-                screen_info.lfb_height = simple_strtoul(this_opt+7, 
NULL, 0);
+                si->lfb_height = simple_strtoul(this_opt+7, NULL, 0);
              else if (!strncmp(this_opt, "width:", 6))
-                screen_info.lfb_width = simple_strtoul(this_opt+6, 
NULL, 0);
+                si->lfb_width = simple_strtoul(this_opt+6, NULL, 0);
              else if (!strcmp(this_opt, "nowc"))
                  mem_flags &= ~EFI_MEMORY_WC;
              else if (!strcmp(this_opt, "nobgrt"))

@@ -310,15 +309,15 @@ static int efifb_setup(char *options)

      return 0;
  }
-static inline bool fb_base_is_valid(void)
+static inline bool fb_base_is_valid(struct screen_info *si)
  {
-    if (screen_info.lfb_base)
+    if (si->lfb_base)
          return true;
-    if (!(screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE))
+    if (!(si->capabilities & VIDEO_CAPABILITY_64BIT_BASE))
          return false;
-    if (screen_info.ext_lfb_base)
+    if (si->ext_lfb_base)
          return true;
      return false;

@@ -329,7 +328,10 @@ static ssize_t name##_show(struct device 

*dev,                \
                 struct device_attribute *attr,        \
                 char *buf)                    \
  {                                    \
-    return sprintf(buf, fmt "\n", (screen_info.lfb_##name));    \
+    struct screen_info *si = dev_get_platdata(dev);            \
+    if (!si)                            \
+        return -ENODEV;                        \
+    return sprintf(buf, fmt "\n", (si->lfb_##name));        \
  }                                    \
  static DEVICE_ATTR_RO(name)

@@ -356,6 +358,7 @@ static u64 bar_offset;

  static int efifb_probe(struct platform_device *dev)
  {
+    struct screen_info *si = &screen_info;
      struct fb_info *info;
      struct efifb_par *par;
      int err, orientation;

@@ -365,48 +368,48 @@ static int efifb_probe(struct platform_device *dev)

      char *option = NULL;
      efi_memory_desc_t md;
-    if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || 
pci_dev_disabled)
+    if (si->orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
          return -ENODEV;
      if (fb_get_options("efifb", &option))
          return -ENODEV;
-    efifb_setup(option);
+    efifb_setup(si, option);
      /* We don't get linelength from UGA Draw Protocol, only from
       * EFI Graphics Protocol.  So if it's not in DMI, and it's not
       * passed in from the user, we really can't use the framebuffer.
       */
-    if (!screen_info.lfb_linelength)
+    if (!si->lfb_linelength)
          return -ENODEV;
-    if (!screen_info.lfb_depth)
-        screen_info.lfb_depth = 32;
-    if (!screen_info.pages)
-        screen_info.pages = 1;
-    if (!fb_base_is_valid()) {
+    if (!si->lfb_depth)
+        si->lfb_depth = 32;
+    if (!si->pages)
+        si->pages = 1;
+    if (!fb_base_is_valid(si)) {
          printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
          return -ENODEV;
      }
      printk(KERN_INFO "efifb: probing for efifb\n");
      /* just assume they're all unset if any are */
-    if (!screen_info.blue_size) {
-        screen_info.blue_size = 8;
-        screen_info.blue_pos = 0;
-        screen_info.green_size = 8;
-        screen_info.green_pos = 8;
-        screen_info.red_size = 8;
-        screen_info.red_pos = 16;
-        screen_info.rsvd_size = 8;
-        screen_info.rsvd_pos = 24;
+    if (!si->blue_size) {
+        si->blue_size = 8;
+        si->blue_pos = 0;
+        si->green_size = 8;
+        si->green_pos = 8;
+        si->red_size = 8;
+        si->red_pos = 16;
+        si->rsvd_size = 8;
+        si->rsvd_pos = 24;
      }

Yeah, Here the efifb modified our local screen_info instance, but this 
is not relevant to your patch.

quoted

-    efifb_fix.smem_start = screen_info.lfb_base;
+    efifb_fix.smem_start = si->lfb_base;
-    if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) {
+    if (si->capabilities & VIDEO_CAPABILITY_64BIT_BASE) {
          u64 ext_lfb_base;
-        ext_lfb_base = (u64)(unsigned long)screen_info.ext_lfb_base 
<< 32;
+        ext_lfb_base = (u64)(unsigned long)si->ext_lfb_base << 32;
          efifb_fix.smem_start |= ext_lfb_base;
      }

@@ -417,10 +420,10 @@ static int efifb_probe(struct platform_device *dev)

          efifb_fix.smem_start = bar_resource->start + bar_offset;
      }
-    efifb_defined.bits_per_pixel = screen_info.lfb_depth;
-    efifb_defined.xres = screen_info.lfb_width;
-    efifb_defined.yres = screen_info.lfb_height;
-    efifb_fix.line_length = screen_info.lfb_linelength;
+    efifb_defined.bits_per_pixel = si->lfb_depth;
+    efifb_defined.xres = si->lfb_width;
+    efifb_defined.yres = si->lfb_height;
+    efifb_fix.line_length = si->lfb_linelength;
      /*   size_vmode -- that is the amount of memory needed for the
       *                 used video mode, i.e. the minimum amount of

@@ -430,7 +433,7 @@ static int efifb_probe(struct platform_device *dev)

      /*   size_total -- all video memory we have. Used for
       *                 entries, ressource allocation and bounds
       *                 checking. */
-    size_total = screen_info.lfb_size;
+    size_total = si->lfb_size;
      if (size_total < size_vmode)
          size_total = size_vmode;

@@ -512,7 +515,7 @@ static int efifb_probe(struct platform_device *dev)

      pr_info("efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
             efifb_defined.xres, efifb_defined.yres,
             efifb_defined.bits_per_pixel, efifb_fix.line_length,
-           screen_info.pages);
+           si->pages);
      efifb_defined.xres_virtual = efifb_defined.xres;
      efifb_defined.yres_virtual = efifb_fix.smem_len /

@@ -526,26 +529,26 @@ static int efifb_probe(struct platform_device *dev)

      efifb_defined.left_margin  = (efifb_defined.xres / 8) & 0xf8;
      efifb_defined.hsync_len    = (efifb_defined.xres / 8) & 0xf8;
-    efifb_defined.red.offset    = screen_info.red_pos;
-    efifb_defined.red.length    = screen_info.red_size;
-    efifb_defined.green.offset  = screen_info.green_pos;
-    efifb_defined.green.length  = screen_info.green_size;
-    efifb_defined.blue.offset   = screen_info.blue_pos;
-    efifb_defined.blue.length   = screen_info.blue_size;
-    efifb_defined.transp.offset = screen_info.rsvd_pos;
-    efifb_defined.transp.length = screen_info.rsvd_size;
+    efifb_defined.red.offset    = si->red_pos;
+    efifb_defined.red.length    = si->red_size;
+    efifb_defined.green.offset  = si->green_pos;
+    efifb_defined.green.length  = si->green_size;
+    efifb_defined.blue.offset   = si->blue_pos;
+    efifb_defined.blue.length   = si->blue_size;
+    efifb_defined.transp.offset = si->rsvd_pos;
+    efifb_defined.transp.length = si->rsvd_size;
      pr_info("efifb: %s: "
             "size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
             "Truecolor",
-           screen_info.rsvd_size,
-           screen_info.red_size,
-           screen_info.green_size,
-           screen_info.blue_size,
-           screen_info.rsvd_pos,
-           screen_info.red_pos,
-           screen_info.green_pos,
-           screen_info.blue_pos);
+           si->rsvd_size,
+           si->red_size,
+           si->green_size,
+           si->blue_size,
+           si->rsvd_pos,
+           si->red_pos,
+           si->green_pos,
+           si->blue_pos);
      efifb_fix.ypanstep  = 0;
      efifb_fix.ywrapstep = 0;

-- 
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)