Hi Am 04.12.23 um 20:53 schrieb Sui Jingfeng:
Hi, On 2023/12/4 17:27, Thomas Zimmermann wrote: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@xxxxxxx> Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>Reviewed-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx> 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--- 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.{ 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.- 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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
