| Message ID | 20220707153952.32264-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | fbdev: Maintain device ownership with aperture helpers | expand |
Hello Thomas, On 7/7/22 17:39, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
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. [...] > 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. [...] > +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 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. > 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.
On 7/7/22 17:39, Thomas Zimmermann wrote: > Move vgag16fb's option parsing into the driver's probe function and > generate the rest of the module's init/exit functions from macros. > Keep the options code, although there are no options defined. > Ah, I see now why you wanted to move the check to the probe function. If is to allow this cleanup then discard that comment from previous patch and I'm OK with the move. Maybe you could comment in patch 02/11 commit message that the check is moved to the probe handler to allow this cleanup as a follow-up patch ? > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- > 1 file changed, 10 insertions(+), 25 deletions(-) > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > index f7c1bb018843..e7767ed50c5b 100644 > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) > > static int vga16fb_probe(struct platform_device *dev) > { > +#ifndef MODULE > + char *option = NULL; > +#endif > struct screen_info *si; > struct fb_info *info; > struct vga16fb_par *par; > int i; > int ret = 0; > > +#ifndef MODULE > + if (fb_get_options("vga16fb", &option)) > + return -ENODEV; > + vga16fb_setup(option); > +#endif > + I would just drop these ifdefery and have the option unconditionally. It seems that's what most fbdev drivers do AFAICT. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
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). 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 >
Hi Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: > On 7/7/22 17:39, Thomas Zimmermann wrote: >> Move vgag16fb's option parsing into the driver's probe function and >> generate the rest of the module's init/exit functions from macros. >> Keep the options code, although there are no options defined. >> > > Ah, I see now why you wanted to move the check to the probe function. If > is to allow this cleanup then discard that comment from previous patch > and I'm OK with the move. > > Maybe you could comment in patch 02/11 commit message that the check is > moved to the probe handler to allow this cleanup as a follow-up patch ? Sure. I mostly wanted to use module_platform_driver(). The options handling is in the way. > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- >> 1 file changed, 10 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >> index f7c1bb018843..e7767ed50c5b 100644 >> --- a/drivers/video/fbdev/vga16fb.c >> +++ b/drivers/video/fbdev/vga16fb.c >> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) >> >> static int vga16fb_probe(struct platform_device *dev) >> { >> +#ifndef MODULE >> + char *option = NULL; >> +#endif >> struct screen_info *si; >> struct fb_info *info; >> struct vga16fb_par *par; >> int i; >> int ret = 0; >> >> +#ifndef MODULE >> + if (fb_get_options("vga16fb", &option)) >> + return -ENODEV; >> + vga16fb_setup(option); >> +#endif >> + > > I would just drop these ifdefery and have the option unconditionally. > It seems that's what most fbdev drivers do AFAICT. Or can we kill it entirely? There are no actual options. Best regards Thomas > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >
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.
On 7/11/22 10:01, Thomas Zimmermann wrote: > Hi > > Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: >> On 7/7/22 17:39, Thomas Zimmermann wrote: >>> Move vgag16fb's option parsing into the driver's probe function and >>> generate the rest of the module's init/exit functions from macros. >>> Keep the options code, although there are no options defined. >>> >> >> Ah, I see now why you wanted to move the check to the probe function. If >> is to allow this cleanup then discard that comment from previous patch >> and I'm OK with the move. >> >> Maybe you could comment in patch 02/11 commit message that the check is >> moved to the probe handler to allow this cleanup as a follow-up patch ? > > Sure. > > I mostly wanted to use module_platform_driver(). The options handling is > in the way. > Yes, I got it when looked at this patch. >> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- >>> 1 file changed, 10 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >>> index f7c1bb018843..e7767ed50c5b 100644 >>> --- a/drivers/video/fbdev/vga16fb.c >>> +++ b/drivers/video/fbdev/vga16fb.c >>> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) >>> >>> static int vga16fb_probe(struct platform_device *dev) >>> { >>> +#ifndef MODULE >>> + char *option = NULL; >>> +#endif >>> struct screen_info *si; >>> struct fb_info *info; >>> struct vga16fb_par *par; >>> int i; >>> int ret = 0; >>> >>> +#ifndef MODULE >>> + if (fb_get_options("vga16fb", &option)) >>> + return -ENODEV; >>> + vga16fb_setup(option); >>> +#endif >>> + >> >> I would just drop these ifdefery and have the option unconditionally. >> It seems that's what most fbdev drivers do AFAICT. > > Or can we kill it entirely? There are no actual options. > That sounds good to me as well.
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 >
On 7/11/22 12:42, Thomas Zimmermann wrote: > Hi Javier [...] >> 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. > Agreed. The driver name is used as the last resort to match a device only if there isn't any device ID table (ACPI, OF, platform, etc) but that's discouraged. Specially when the same driver supports different devices as it's the case for this driver.
Fbdev firmware drivers acquire ownership of framebuffer I/O ranges and hand them over to native drivers during the boot process. Re-implement this mechanism with aperture helpers and remove the respective fbdev code. This change allows to perform hand-over from DRM firmware drivers. In a later patchset, device ownership can be moved from DRM and fbdev entirely into aperture helpers. Patches 1 and 4 are cleanups. Patches 2 and 3 integrate EGA/VGA support into sysfb, although it's not clear if the x86 architecture code actually still supports VGA graphics mode. Patches 5 to 10 replace fbdev's ownership management with aperture helpers. This includes removal of conflicting framebuffer drivers, removal of conflicting VGA drivers and registration of fbdev firmware devices. Notably, many PCI-based fbdev drivers failed to remove firmware devices until now; and therefore probably haven't worked correctly for some time. Patch 11 removes the implementation of fbdev ownership management. The patchset has been tested by handing over device ownership between firmware and native drivers of DRM and fbdev in various combinations. Thomas Zimmermann (11): fbdev: Remove trailing whitespaces fbdev/vga16fb: Create EGA/VGA devices in sysfb code fbdev/vga16fb: Auto-generate module init/exit code fbdev/core: Remove remove_conflicting_pci_framebuffers() fbdev: Convert drivers to aperture helpers fbdev: Remove conflicting devices on PCI bus video/aperture: Disable and unregister sysfb devices via aperture helpers video: Provide constants for VGA I/O range video/aperture: Remove conflicting VGA devices, if any fbdev: Acquire framebuffer apertures for firmware devices fbdev: Remove conflict-handling code drivers/firmware/sysfb.c | 4 + drivers/staging/sm750fb/sm750.c | 15 +- drivers/video/aperture.c | 69 ++-- drivers/video/fbdev/arkfb.c | 5 + drivers/video/fbdev/asiliantfb.c | 5 + drivers/video/fbdev/aty/aty128fb.c | 57 ++-- drivers/video/fbdev/aty/atyfb_base.c | 7 +- drivers/video/fbdev/aty/radeon_base.c | 83 +++-- drivers/video/fbdev/carminefb.c | 5 + drivers/video/fbdev/chipsfb.c | 13 +- drivers/video/fbdev/cirrusfb.c | 5 + drivers/video/fbdev/core/fbmem.c | 176 ++--------- drivers/video/fbdev/cyber2000fb.c | 5 + drivers/video/fbdev/geode/gx1fb_core.c | 5 + drivers/video/fbdev/geode/gxfb_core.c | 5 + drivers/video/fbdev/geode/lxfb_core.c | 5 + drivers/video/fbdev/gxt4500.c | 5 + drivers/video/fbdev/hyperv_fb.c | 6 +- drivers/video/fbdev/i740fb.c | 5 + drivers/video/fbdev/i810/i810_main.c | 315 ++++++++++--------- drivers/video/fbdev/imsttfb.c | 36 ++- drivers/video/fbdev/intelfb/intelfbdrv.c | 5 + drivers/video/fbdev/kyro/fbdev.c | 5 + drivers/video/fbdev/matrox/matroxfb_base.c | 5 + drivers/video/fbdev/mb862xx/mb862xxfbdrv.c | 5 + drivers/video/fbdev/neofb.c | 41 +-- drivers/video/fbdev/nvidia/nvidia.c | 7 +- drivers/video/fbdev/pm2fb.c | 5 + drivers/video/fbdev/pm3fb.c | 5 + drivers/video/fbdev/pvr2fb.c | 5 + drivers/video/fbdev/riva/fbdev.c | 67 ++-- drivers/video/fbdev/s3fb.c | 5 + drivers/video/fbdev/savage/savagefb_driver.c | 5 + drivers/video/fbdev/sis/sis_main.c | 5 + drivers/video/fbdev/skeletonfb.c | 210 +++++++------ drivers/video/fbdev/sm712fb.c | 5 + drivers/video/fbdev/sstfb.c | 43 +-- drivers/video/fbdev/sunxvr2500.c | 5 + drivers/video/fbdev/sunxvr500.c | 5 + drivers/video/fbdev/tdfxfb.c | 5 + drivers/video/fbdev/tgafb.c | 17 +- drivers/video/fbdev/tridentfb.c | 5 + drivers/video/fbdev/vermilion/vermilion.c | 7 +- drivers/video/fbdev/vga16fb.c | 191 +++++------ drivers/video/fbdev/via/via-core.c | 5 + drivers/video/fbdev/vt8623fb.c | 5 + include/linux/fb.h | 4 - include/video/vga.h | 20 +- 48 files changed, 788 insertions(+), 735 deletions(-) base-commit: 11d480026e922adacd274306728adb6df6dd262a prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24