Hi Javier Am 11.07.22 um 11:54 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 7/11/22 09:58, Thomas Zimmermann wrote: >> Hi Javier, >> >> I'll try to give the motivation of this patch below. I known it's rather >> hypothetical as the VGA driver is probably not used much. >> >> Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas: >>> Hello Thomas, >>> >>> On 7/7/22 17:39, Thomas Zimmermann wrote: >>>> Move the device-creation from vga16fb to sysfb code. Move the few >>>> extra videomode checks into vga16fb's probe function. >>>> >>>> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC >>>> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except >>>> for some MIPS systems. It's not clear if the vga16fb driver actually >>>> works in practice. >>>> >>>> Signed-off-by: Thomas Zimmermann >>>> --- >>>> drivers/firmware/sysfb.c | 4 +++ >>>> drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------ >>>> 2 files changed, 32 insertions(+), 31 deletions(-) >>>> >>>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c >>>> index 1f276f108cc9..3fd3563d962b 100644 >>>> --- a/drivers/firmware/sysfb.c >>>> +++ b/drivers/firmware/sysfb.c >>>> @@ -94,6 +94,10 @@ static __init int sysfb_init(void) >>>> name = "efi-framebuffer"; >>>> else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB) >>>> name = "vesa-framebuffer"; >>>> + else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC) >>>> + name = "vga-framebuffer"; >>>> + else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC) >>>> + >>> >>> I wonder if we really need to do this distinction or could just have a single >>> platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the >>> same fbdev driver is bound against both platform devices. >> >> With the current driver, we don't strictly need to distinguish. But the >> sysfb code is the one we care about. So I wanted it to be clear and >> nicely looking. All the mode tests, etc depend on the driver (which no >> one cares about). >> > > Right. That is a good point. We don't want to leak a driver implementation > detail in the sysfb code. And in theory there could be for example a DRM > driver for EGA and one for VGA. > >> There's also a difference in hardware features between EGA and VGA. Most >> notably, VGA has much better color support. >> > > Yes, I know the differences. My point was that the orig_video_isVGA was > used to make the distinction and that the same driver supported both, but > as you said that may not be the case and separate drivers could be used. > >>> >>> [...] >>> >>>> static int vga16fb_probe(struct platform_device *dev) >>>> { >>>> + struct screen_info *si; >>>> struct fb_info *info; >>>> struct vga16fb_par *par; >>>> int i; >>>> int ret = 0; >>>> >>>> + si = dev_get_platdata(&dev->dev); >>>> + if (!si) >>>> + return -ENODEV; >>>> + >>>> + ret = check_mode_supported(si); >>>> + if (ret) >>>> + return ret; >>>> + >>> >>> What do you see as the advantage of moving the check to the driver's probe? >>> >>> Because after this change the platform driver will be registered but for no >>> reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC. >> >> The driver only supports very specific modes, which may not be what >> screen_info detected. Note that VGAC/EGAC can also refer to text-mode >> buffers. The current vgacon could be turned into a platform driver that >> adopts such a text buffer and integrates it with aperture helpers. >> > > Yes, and also there's also the monochrome variant of VGA and EGA (VGAM/EGAM). > > Should we also make that distinction or for example "vga-framebuffer" should > handle both color and monochrome variants if at some point vgacon is turned > into a fbdev or DRM driver ? > >>> >>> [...] >>> >>>> +static const struct platform_device_id vga16fb_driver_id_table[] = { >>>> + {"ega-framebuffer", 0}, >>>> + {"vga-framebuffer", 0}, >>>> + { } >>>> +}; >>>> + >>> >>> The fact that the two entries don't have a platform data is an indication for >> >> The name is the indication. I know that vga16 doesn't treat them >> differently. >> > > Yes, my point was that the platform device name used to bind is an internal > Linux interface that could be changed later if needed. But I understand your > point and since the platform device names are exposed to user-space, makes > more sense for them to reflect what devices are bound even when the existing > driver doesn't treat them differently. > >>> me that we could just consolidate in a single "vga16-framebuffer" or smt. I >>> know that this won't be consistent with efi, vesa, etc but I don't think is >>> that important and also quite likely we will get rid of this driver and the >>> platform device registration soon. Since as you said, it's unclear that is >>> even used. >> >> There's mips code in the arch/ directory that appears to setup >> screen_info in the correct way. I can't say whether that's still useful >> to anyone. On x86, I could set a VGA mode on the kernel command line, >> but screen_info's isVGA only contained '1'. It might be possible to fix >> this easily by setting the right values in vga_probe(). [1] I simply >> don't have the time to provide a patch and deal with the potential >> fallout of such a change. >> > > Indeed. This seems to be a remnant from the time when isVGA was just a bool > and then at some point the field semantics was extended to denote the mode > but the existing users weren't fixed (nor the field named to reflect this). > > Probably should be cleaned up at some point but unsure if the churn would > be worth it. > >>> >>>> static struct platform_driver vga16fb_driver = { >>>> .probe = vga16fb_probe, >>>> .remove = vga16fb_remove, >>>> .driver = { >>>> - .name = "vga16fb", >>>> + .name = "vga-framebuffer", >>>> }, >>> >>> Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK. >> >> VESA is something else than VGA. Setting a VESA mode (done via INT 10h >> IIRC) and then fiddling with VGA state will likely produce broken output >> on the screen. >> > > Technically it is something else but Linux conflates them in many places. For > example, as you mentioned one can change the VESA modes using the "vga" param > (which confusingly leads to the use of vesafb+fbcon driver instead of vgacon). > > That's why I think that "vga-framebuffer" as driver name would be misleading. > Specially since it would also bind to the "ega-framebuffer" platform device. I messed up device and driver name, such that misunderstood your remark. As we use the id_table field for matching devices, the driver name doesn't matter. [1] So let's keep the driver name as vga16fb. The change above must have been left over from an earlier prototype patch, I guess. Best regards Thomas [1] https://elixir.bootlin.com/linux/v5.18.10/source/drivers/base/platform.c#L1365 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev