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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 7356 bytes --]

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 <tzimmermann@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).
>>
> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]