[PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 11 10:42:29 UTC 2022
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 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).
>>
>
> 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
-------------- 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/9c1b1833/attachment.sig>
More information about the dri-devel
mailing list