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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux