[PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 11 07:58:58 UTC 2022
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 at suse.de>
>> ---
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220711/b4077498/attachment.sig>
More information about the dri-devel
mailing list