| Message ID | 20220107110723.323276-2-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | video: A couple of fixes for the vga16fb driver | expand |
Hi Javier, On Fri, Jan 7, 2022 at 9:00 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > The vga16fb framebuffer driver supports both Enhanced Graphics Adapter > (EGA) and Video Graphics Array (VGA) 16 color graphic cards. > > But the logic to check whether the EGA or VGA standard are used is not > correct. It just checks if screen_info.orig_video_isVGA is set, but it > should check if is set to VIDEO_TYPE_VGAC instead. > > This means that it assumes to be VGA even if is set to VIDEO_TYPE_EGAC. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Thanks for your patch! > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -1332,7 +1332,7 @@ static int vga16fb_probe(struct platform_device *dev) > printk(KERN_INFO "vga16fb: mapped to 0x%p\n", info->screen_base); > par = info->par; > > - par->isVGA = screen_info.orig_video_isVGA; > + par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC; All non-x86 architectures (except for 2 MIPS platforms) treat orig_video_isVGA as a boolean flag, and just assign 1 to it. Hence this change would break them. > par->palette_blanked = 0; > par->vesa_blanked = 0; > 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
Groetje, Geert, Geert Uytterhoeven wrote: > >> - par->isVGA = screen_info.orig_video_isVGA; >> + par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC; > All non-x86 architectures (except for 2 MIPS platforms) treat > orig_video_isVGA as a boolean flag, and just assign 1 to it. > Hence this change would break them. I see a bit of a conflict with using orig_video_isVGA as a boolean. All the modern architecture-agnostic driver code, such as sysfb, sysfb_simplefb, and efifb, all use and expect orig_video_isVGA to be an integer. On the other hand, the VGA driver for XEN first sets orig_video_isVGA = 1 (boolean), and then VIDEO_TYPE_VLFB or VIDEO_TYPE_EFI (integer). Overloading the definition for orig_video_isVGA to be both boolean and integer - within the same file - seems like a recipe for bugs to me. That said, I think that wrapping the par->isVGA code, above, within a check for CONFIG_X86 seems safe and expedient. But I would be much happier if the non-x86 architectures would set it to a proper integer value (even if fake) that coincidentally satisfies boolean "true", say VIDEO_TYPE_VGAC; that way, there would be no confusion as to data type in all the more recent architecture-agnostic framebuffer code. Kris
Hello Geert and Kara, On 1/9/22 21:20, Kris Karas (Bug reporting) wrote: > Groetje, Geert, > > Geert Uytterhoeven wrote: >> >>> - par->isVGA = screen_info.orig_video_isVGA; >>> + par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC; >> All non-x86 architectures (except for 2 MIPS platforms) treat >> orig_video_isVGA as a boolean flag, and just assign 1 to it. >> Hence this change would break them. > > I see a bit of a conflict with using orig_video_isVGA as a boolean. All > the modern architecture-agnostic driver code, such as sysfb, > sysfb_simplefb, and efifb, all use and expect orig_video_isVGA to be an > integer. On the other hand, the VGA driver for XEN first sets > orig_video_isVGA = 1 (boolean), and then VIDEO_TYPE_VLFB or > VIDEO_TYPE_EFI (integer). Overloading the definition for > orig_video_isVGA to be both boolean and integer - within the same file - > seems like a recipe for bugs to me. > Agreed with Kara on this. I believe the non-x86 arches should be fixed to set it to the correct value. > That said, I think that wrapping the par->isVGA code, above, within a > check for CONFIG_X86 seems safe and expedient. But I would be much > happier if the non-x86 architectures would set it to a proper integer > value (even if fake) that coincidentally satisfies boolean "true", say > VIDEO_TYPE_VGAC; that way, there would be no confusion as to data type > in all the more recent architecture-agnostic framebuffer code. > Yes, I'll post a v2 to do that but hopefully the CONFIG_X86 guard could be dropped in the future once all the architectures are fixed. > Kris > Best regards,
diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c index e2757ff1c23d..3347c9b6a332 100644 --- a/drivers/video/fbdev/vga16fb.c +++ b/drivers/video/fbdev/vga16fb.c @@ -1332,7 +1332,7 @@ static int vga16fb_probe(struct platform_device *dev) printk(KERN_INFO "vga16fb: mapped to 0x%p\n", info->screen_base); par = info->par; - par->isVGA = screen_info.orig_video_isVGA; + par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC; par->palette_blanked = 0; par->vesa_blanked = 0;
The vga16fb framebuffer driver supports both Enhanced Graphics Adapter (EGA) and Video Graphics Array (VGA) 16 color graphic cards. But the logic to check whether the EGA or VGA standard are used is not correct. It just checks if screen_info.orig_video_isVGA is set, but it should check if is set to VIDEO_TYPE_VGAC instead. This means that it assumes to be VGA even if is set to VIDEO_TYPE_EGAC. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/video/fbdev/vga16fb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)