[PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Thomas Zimmermann]

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>