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 <tzimmermann@xxxxxxx> --- 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).
There's also a difference in hardware features between EGA and VGA. Most notably, VGA has much better color support.
[...]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.
[...]+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.
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.
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.
Best regards Thomas[1] https://elixir.bootlin.com/linux/v5.18.10/source/arch/x86/boot/video-vga.c#L231
-- 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
