| Message ID | 20230510110557.14343-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand |
Hi, Thomas I love your patch, yet something to improve: On 2023/5/10 19:05, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> > Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> > --- > drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++--- > drivers/video/fbdev/matrox/matroxfb_base.h | 4 ++-- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c > index 9cb0685feddd..ce51227798a1 100644 > --- a/drivers/video/fbdev/matrox/matroxfb_accel.c > +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c > @@ -88,7 +88,7 @@ > > static inline void matrox_cfb4_pal(u_int32_t* pal) { > unsigned int i; > - > + > for (i = 0; i < 16; i++) { > pal[i] = i * 0x11111111U; > } > @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { > > static inline void matrox_cfb8_pal(u_int32_t* pal) { > unsigned int i; > - > + > for (i = 0; i < 16; i++) { > pal[i] = i * 0x01010101U; > } > @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx, > /* Tell... well, why bother... */ > while (height--) { > size_t i; > - > + > for (i = 0; i < step; i += 4) { > /* Hope that there are at least three readable bytes beyond the end of bitmap */ > fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr); > diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h > index 958be6805f87..c93c69bbcd57 100644 > --- a/drivers/video/fbdev/matrox/matroxfb_base.h > +++ b/drivers/video/fbdev/matrox/matroxfb_base.h > @@ -301,9 +301,9 @@ struct matrox_altout { > int (*verifymode)(void* altout_dev, u_int32_t mode); > int (*getqueryctrl)(void* altout_dev, > struct v4l2_queryctrl* ctrl); Noticed that there are plenty of coding style problems in matroxfb_base.h, why you only fix a few of them? Take this two line as an example, shouldn't they be fixed also as following? int (*verifymode)(void *altout_dev, u_int32_t mode); int (*getqueryctrl)(void *altout_dev, struct v4l2_queryctrl *ctrl); > - int (*getctrl)(void* altout_dev, > + int (*getctrl)(void *altout_dev, > struct v4l2_control* ctrl); > - int (*setctrl)(void* altout_dev, > + int (*setctrl)(void *altout_dev, > struct v4l2_control* ctrl); > }; >
Hi Am 10.05.23 um 20:20 schrieb Sui Jingfeng: > Hi, Thomas > > > I love your patch, yet something to improve: > > > On 2023/5/10 19:05, Thomas Zimmermann wrote: >> Fix coding style. No functional changes. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reviewed-by: Arnd Bergmann <arnd@arndb.de> >> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> >> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> >> --- >> drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++--- >> drivers/video/fbdev/matrox/matroxfb_base.h | 4 ++-- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c >> b/drivers/video/fbdev/matrox/matroxfb_accel.c >> index 9cb0685feddd..ce51227798a1 100644 >> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c >> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c >> @@ -88,7 +88,7 @@ >> static inline void matrox_cfb4_pal(u_int32_t* pal) { >> unsigned int i; >> - >> + >> for (i = 0; i < 16; i++) { >> pal[i] = i * 0x11111111U; >> } >> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { >> static inline void matrox_cfb8_pal(u_int32_t* pal) { >> unsigned int i; >> - >> + >> for (i = 0; i < 16; i++) { >> pal[i] = i * 0x01010101U; >> } >> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct >> matrox_fb_info *minfo, u_int32_t fgx, >> /* Tell... well, why bother... */ >> while (height--) { >> size_t i; >> - >> + >> for (i = 0; i < step; i += 4) { >> /* Hope that there are at least three readable >> bytes beyond the end of bitmap */ >> fb_writel(get_unaligned((u_int32_t*)(chardata + >> i)),mmio.vaddr); >> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h >> b/drivers/video/fbdev/matrox/matroxfb_base.h >> index 958be6805f87..c93c69bbcd57 100644 >> --- a/drivers/video/fbdev/matrox/matroxfb_base.h >> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h >> @@ -301,9 +301,9 @@ struct matrox_altout { >> int (*verifymode)(void* altout_dev, u_int32_t mode); >> int (*getqueryctrl)(void* altout_dev, >> struct v4l2_queryctrl* ctrl); > > Noticed that there are plenty of coding style problems in matroxfb_base.h, > > why you only fix a few of them? Take this two line as an example, > shouldn't > > they be fixed also as following? I configured my text editor to remove trailing whitespaces automatically. That keeps my own patches free of them. But the editor removes all trailing whitespaces, including those that have been there before. If I encounter such a case, I split out the whitespace fix and submit it separately. But the work I do within fbdev is mostly for improving DRM. For the other issues in this file, I don't think that matroxfb should even be around any longer. Fbdev has been deprecated for a long time. But a small number of drivers are still in use and we still need its framebuffer console. So someone should either put significant effort into maintaining fbdev, or it should be phased out. But neither is happening. Best regards Thomas > > > int (*verifymode)(void *altout_dev, u_int32_t mode); > int (*getqueryctrl)(void *altout_dev, > struct v4l2_queryctrl *ctrl); > > >> - int (*getctrl)(void* altout_dev, >> + int (*getctrl)(void *altout_dev, >> struct v4l2_control* ctrl); >> - int (*setctrl)(void* altout_dev, >> + int (*setctrl)(void *altout_dev, >> struct v4l2_control* ctrl); >> };
Hi On 2023/5/11 15:55, Thomas Zimmermann wrote: > Hi > > Am 10.05.23 um 20:20 schrieb Sui Jingfeng: >> Hi, Thomas >> >> >> I love your patch, yet something to improve: >> >> >> On 2023/5/10 19:05, Thomas Zimmermann wrote: >>> Fix coding style. No functional changes. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Reviewed-by: Arnd Bergmann <arnd@arndb.de> >>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> >>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> >>> --- >>> drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++--- >>> drivers/video/fbdev/matrox/matroxfb_base.h | 4 ++-- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c >>> b/drivers/video/fbdev/matrox/matroxfb_accel.c >>> index 9cb0685feddd..ce51227798a1 100644 >>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c >>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c >>> @@ -88,7 +88,7 @@ >>> static inline void matrox_cfb4_pal(u_int32_t* pal) { >>> unsigned int i; >>> - >>> + >>> for (i = 0; i < 16; i++) { >>> pal[i] = i * 0x11111111U; >>> } >>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { >>> static inline void matrox_cfb8_pal(u_int32_t* pal) { >>> unsigned int i; >>> - >>> + >>> for (i = 0; i < 16; i++) { >>> pal[i] = i * 0x01010101U; >>> } >>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct >>> matrox_fb_info *minfo, u_int32_t fgx, >>> /* Tell... well, why bother... */ >>> while (height--) { >>> size_t i; >>> - >>> + >>> for (i = 0; i < step; i += 4) { >>> /* Hope that there are at least three readable >>> bytes beyond the end of bitmap */ >>> fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr); >>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h >>> b/drivers/video/fbdev/matrox/matroxfb_base.h >>> index 958be6805f87..c93c69bbcd57 100644 >>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h >>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h >>> @@ -301,9 +301,9 @@ struct matrox_altout { >>> int (*verifymode)(void* altout_dev, u_int32_t mode); >>> int (*getqueryctrl)(void* altout_dev, >>> struct v4l2_queryctrl* ctrl); >> >> Noticed that there are plenty of coding style problems in >> matroxfb_base.h, >> >> why you only fix a few of them? Take this two line as an example, >> shouldn't >> >> they be fixed also as following? > > I configured my text editor to remove trailing whitespaces > automatically. That keeps my own patches free of them. But the editor > removes all trailing whitespaces, including those that have been there > before. If I encounter such a case, I split out the whitespace fix and > submit it separately. > > But the work I do within fbdev is mostly for improving DRM. For the > other issues in this file, I don't think that matroxfb should even be > around any longer. Fbdev has been deprecated for a long time. But a > small number of drivers are still in use and we still need its > framebuffer console. So someone should either put significant effort > into maintaining fbdev, or it should be phased out. But neither is > happening. > Ok, no problem, that sound fine and reasonable then. The lines being modified has trailing whitespaces. And I tested your patch again last night on loongarch and mips platform. It still works in my testing case. > Best regards > Thomas > >> >> >> int (*verifymode)(void *altout_dev, u_int32_t mode); >> int (*getqueryctrl)(void *altout_dev, >> struct v4l2_queryctrl *ctrl); >> >> >>> - int (*getctrl)(void* altout_dev, >>> + int (*getctrl)(void *altout_dev, >>> struct v4l2_control* ctrl); >>> - int (*setctrl)(void* altout_dev, >>> + int (*setctrl)(void *altout_dev, >>> struct v4l2_control* ctrl); >>> }; >
On 5/11/23 09:55, Thomas Zimmermann wrote: > Hi > > Am 10.05.23 um 20:20 schrieb Sui Jingfeng: >> Hi, Thomas >> >> >> I love your patch, yet something to improve: >> >> >> On 2023/5/10 19:05, Thomas Zimmermann wrote: >>> Fix coding style. No functional changes. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Reviewed-by: Arnd Bergmann <arnd@arndb.de> >>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> >>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> >>> --- >>> drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++--- >>> drivers/video/fbdev/matrox/matroxfb_base.h | 4 ++-- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c >>> index 9cb0685feddd..ce51227798a1 100644 >>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c >>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c >>> @@ -88,7 +88,7 @@ >>> static inline void matrox_cfb4_pal(u_int32_t* pal) { >>> unsigned int i; >>> - >>> + >>> for (i = 0; i < 16; i++) { >>> pal[i] = i * 0x11111111U; >>> } >>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { >>> static inline void matrox_cfb8_pal(u_int32_t* pal) { >>> unsigned int i; >>> - >>> + >>> for (i = 0; i < 16; i++) { >>> pal[i] = i * 0x01010101U; >>> } >>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx, >>> /* Tell... well, why bother... */ >>> while (height--) { >>> size_t i; >>> - >>> + >>> for (i = 0; i < step; i += 4) { >>> /* Hope that there are at least three readable bytes beyond the end of bitmap */ >>> fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr); >>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h >>> index 958be6805f87..c93c69bbcd57 100644 >>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h >>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h >>> @@ -301,9 +301,9 @@ struct matrox_altout { >>> int (*verifymode)(void* altout_dev, u_int32_t mode); >>> int (*getqueryctrl)(void* altout_dev, >>> struct v4l2_queryctrl* ctrl); >> >> Noticed that there are plenty of coding style problems in matroxfb_base.h, >> >> why you only fix a few of them? Take this two line as an example, shouldn't >> >> they be fixed also as following? > > I configured my text editor to remove trailing whitespaces > automatically. That keeps my own patches free of them. But the > editor removes all trailing whitespaces, including those that have > been there before. If I encounter such a case, I split out the > whitespace fix and submit it separately. > > But the work I do within fbdev is mostly for improving DRM. Sure. > For the > other issues in this file, I don't think that matroxfb should even be > around any longer. Fbdev has been deprecated for a long time. But a > small number of drivers are still in use and we still need its > framebuffer console. So someone should either put significant effort > into maintaining fbdev, or it should be phased out. But neither is > happening. You're wrong. You don't mention that for most older machines DRM isn't an acceptable way to go due to it's limitations, e.g. it's low-speed due to missing 2D-acceleration for older cards and and it's incapability to change screen resolution at runtime (just to name two of the bigger limitations here). So, unless we somehow find a good way to move such drivers over to DRM (with a set of minimal 2D acceleration), they are still important. Actually, I just did test matroxfb and pm2fb successfully a few days back, and they worked. For some smaller issues I've prepared patches, which are on hold due conflicts with your latest file-move-around- and whitespace-changes which are partly in drm-misc. And I do have some upcoming additional patches for console support. Helge
Hi Helge, On Thu, May 11, 2023 at 3:05 PM Helge Deller <deller@gmx.de> wrote: > On 5/11/23 09:55, Thomas Zimmermann wrote: > > But the work I do within fbdev is mostly for improving DRM. > > Sure. > > > For the > > other issues in this file, I don't think that matroxfb should even be > > around any longer. Fbdev has been deprecated for a long time. But a > > small number of drivers are still in use and we still need its > > framebuffer console. So someone should either put significant effort > > into maintaining fbdev, or it should be phased out. But neither is > > happening. > > You're wrong. > > You don't mention that for most older machines DRM isn't an acceptable > way to go due to it's limitations, e.g. it's low-speed due to missing > 2D-acceleration for older cards and and it's incapability to change screen > resolution at runtime (just to name two of the bigger limitations here). > So, unless we somehow find a good way to move such drivers over to DRM > (with a set of minimal 2D acceleration), they are still important. DRM can change resolution at runtime, just not through the fbdev API. Or do you mean the resolution of the text console, akin to "fbset <mode>"? I have to admit I do not know if there is a command line tool to do that... Gr{oetje,eeting}s, Geert
Hi Am 11.05.23 um 15:05 schrieb Helge Deller: > On 5/11/23 09:55, Thomas Zimmermann wrote: >> Hi >> >> Am 10.05.23 um 20:20 schrieb Sui Jingfeng: >>> Hi, Thomas >>> >>> >>> I love your patch, yet something to improve: >>> >>> >>> On 2023/5/10 19:05, Thomas Zimmermann wrote: >>>> Fix coding style. No functional changes. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de> >>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> >>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> >>>> --- >>>> drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++--- >>>> drivers/video/fbdev/matrox/matroxfb_base.h | 4 ++-- >>>> 2 files changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c >>>> b/drivers/video/fbdev/matrox/matroxfb_accel.c >>>> index 9cb0685feddd..ce51227798a1 100644 >>>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c >>>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c >>>> @@ -88,7 +88,7 @@ >>>> static inline void matrox_cfb4_pal(u_int32_t* pal) { >>>> unsigned int i; >>>> - >>>> + >>>> for (i = 0; i < 16; i++) { >>>> pal[i] = i * 0x11111111U; >>>> } >>>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { >>>> static inline void matrox_cfb8_pal(u_int32_t* pal) { >>>> unsigned int i; >>>> - >>>> + >>>> for (i = 0; i < 16; i++) { >>>> pal[i] = i * 0x01010101U; >>>> } >>>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct >>>> matrox_fb_info *minfo, u_int32_t fgx, >>>> /* Tell... well, why bother... */ >>>> while (height--) { >>>> size_t i; >>>> - >>>> + >>>> for (i = 0; i < step; i += 4) { >>>> /* Hope that there are at least three readable >>>> bytes beyond the end of bitmap */ >>>> fb_writel(get_unaligned((u_int32_t*)(chardata >>>> + i)),mmio.vaddr); >>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h >>>> b/drivers/video/fbdev/matrox/matroxfb_base.h >>>> index 958be6805f87..c93c69bbcd57 100644 >>>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h >>>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h >>>> @@ -301,9 +301,9 @@ struct matrox_altout { >>>> int (*verifymode)(void* altout_dev, u_int32_t mode); >>>> int (*getqueryctrl)(void* altout_dev, >>>> struct v4l2_queryctrl* ctrl); >>> >>> Noticed that there are plenty of coding style problems in >>> matroxfb_base.h, >>> >>> why you only fix a few of them? Take this two line as an example, >>> shouldn't >>> >>> they be fixed also as following? >> >> I configured my text editor to remove trailing whitespaces >> automatically. That keeps my own patches free of them. But the >> editor removes all trailing whitespaces, including those that have >> been there before. If I encounter such a case, I split out the >> whitespace fix and submit it separately. >> >> But the work I do within fbdev is mostly for improving DRM. > > Sure. > >> For the >> other issues in this file, I don't think that matroxfb should even be >> around any longer. Fbdev has been deprecated for a long time. But a >> small number of drivers are still in use and we still need its >> framebuffer console. So someone should either put significant effort >> into maintaining fbdev, or it should be phased out. But neither is >> happening. > > You're wrong. I'm not. I don't claim that these drivers are all broken. But fbdev as a whole is bit-rotting and no one attempts to address this. There are several recent examples of this: * I recently send out a 100-patches series to improve parameter parsing and avoid memory leaks. That got shot down. I didn't attempt to support parameter parsing for module builds. * There's been a 15-yrs old bug in fbdev's read/write where they return an incorrect value. * See the other discussion on this patchset on the state of hitfb. * The fbdev code I recently cleaned up had bugs in how it uses some of fbdev's basic building blocks (see the screen_base/screen_buffer confusion). * <asm-generic/fb.h> has been in the tree since 2009 and no one attempted to include it until now. None of this is a sign of good maintenance. As I've worked on DRM's fbdev emulation a lot, I try to be a good kernel citizen and clean up in fbdev as well when I see a problem. But I'd really like to see most of these drivers being moved into staging and deleted soon afterwards. Users will complain about those drivers that are really still required. Those might be worth to spend effort on. > > You don't mention that for most older machines DRM isn't an acceptable > way to go due to it's limitations, e.g. it's low-speed due to missing > 2D-acceleration for older cards and and it's incapability to change screen > resolution at runtime (just to name two of the bigger limitations here). You can change resolution at runtime; just not through fbdev ioctls. There's no technical limitation here. No one found any use for this, so it's not there. > So, unless we somehow find a good way to move such drivers over to DRM > (with a set of minimal 2D acceleration), they are still important. 2d acceleration is mostly useful for the framebuffer console. You can do that with DRM and drivers have (nouveau). It just didn't make a meaningful difference in most cases. Best regards Thomas > > Actually, I just did test matroxfb and pm2fb successfully a few days > back, and > they worked. For some smaller issues I've prepared patches, which are on > hold > due conflicts with your latest file-move-around- and whitespace-changes > which are partly > in drm-misc. > And I do have some upcoming additional patches for console support. > > Helge
On 5/11/23 15:10, Geert Uytterhoeven wrote: > Hi Helge, > > On Thu, May 11, 2023 at 3:05 PM Helge Deller <deller@gmx.de> wrote: >> On 5/11/23 09:55, Thomas Zimmermann wrote: >>> But the work I do within fbdev is mostly for improving DRM. >> >> Sure. >> >>> For the >>> other issues in this file, I don't think that matroxfb should even be >>> around any longer. Fbdev has been deprecated for a long time. But a >>> small number of drivers are still in use and we still need its >>> framebuffer console. So someone should either put significant effort >>> into maintaining fbdev, or it should be phased out. But neither is >>> happening. >> >> You're wrong. >> >> You don't mention that for most older machines DRM isn't an acceptable >> way to go due to it's limitations, e.g. it's low-speed due to missing >> 2D-acceleration for older cards and and it's incapability to change screen >> resolution at runtime (just to name two of the bigger limitations here). >> So, unless we somehow find a good way to move such drivers over to DRM >> (with a set of minimal 2D acceleration), they are still important. > > DRM can change resolution at runtime, Right, sure. > just not through the fbdev API. ... and sadly the simpledrm-based drivers neither. > Or do you mean the resolution of the text console, akin to > "fbset <mode>"? yes. > I have to admit I do not know if there is a command > line tool to do that... Helge
On 5/11/23 16:27, Thomas Zimmermann wrote: >>> But the work I do within fbdev is mostly for improving DRM. >> >> Sure. >> >>> For the >>> other issues in this file, I don't think that matroxfb should even be >>> around any longer. Fbdev has been deprecated for a long time. But a >>> small number of drivers are still in use and we still need its >>> framebuffer console. So someone should either put significant effort >>> into maintaining fbdev, or it should be phased out. But neither is >>> happening. >> >> You're wrong. > > I'm not. I don't claim that these drivers are all broken. But fbdev > as a whole is bit-rotting and no one attempts to address this. There > are several recent examples of this: > > * I recently send out a 100-patches series to improve parameter > parsing and avoid memory leaks. That got shot down. I didn't attempt > to support parameter parsing for module builds. Your work is appreciated and it wasn't shot down, but it wasn't perfect either. > * There's been a 15-yrs old bug in fbdev's read/write where they > return an incorrect value. ? > * See the other discussion on this patchset on the state of hitfb. > > * The fbdev code I recently cleaned up had bugs in how it uses some > of fbdev's basic building blocks (see the screen_base/screen_buffer > confusion). As you said... some (little-used/outdated) drivers may have issues which of course show up if one starts to clean up, as you do. On a per-driver basis it can make sense to drop a specific driver. > * <asm-generic/fb.h> has been in the tree since 2009 and no one > attempted to include it until now. > > None of this is a sign of good maintenance. > As I've worked on DRM's fbdev emulation a lot, I try to be a good > kernel citizen and clean up in fbdev as well when I see a problem.> But I'd really like to see most of these drivers being moved into > staging and deleted soon afterwards. Users will complain about those > drivers that are really still required. Those might be worth to spend > effort on. I'd really like to see a way forward and get the required drivers over to DRM, e.g. based on your simpledrm driver. If there is a way to get basic on-screen 2D bitblt and fillrect support, it would drop the need for most of the fbdev drivers. The current way of bitblt'ing from a buffer on regular basis istoo slow for such older cards. Even on new hardware in emulators there is a big slowdown visible. >> You don't mention that for most older machines DRM isn't an >> acceptable way to go due to it's limitations, e.g. it's low-speed >> due to missing 2D-acceleration for older cards and and it's >> incapability to change screen resolution at runtime (just to name >> two of the bigger limitations here). > > You can change resolution at runtime; just not through fbdev ioctls. > There's no technical limitation here. No one found any use for this, > so it's not there. fbdev drivers would need that when ported to DRM. >> So, unless we somehow find a good way to move such drivers over to >> DRM (with a set of minimal 2D acceleration), they are still >> important. > > 2d acceleration is mostly useful for the framebuffer console. and X11 > You can > do that with DRM and drivers have (nouveau). It just didn't make a > meaningful difference in most cases. if nouveau got it, can't it be done for simpledrm in a generic way too? >> Actually, I just did test matroxfb and pm2fb successfully a few >> days back, and they worked. For some smaller issues I've prepared >> patches, which are on hold due conflicts with your latest >> file-move-around- and whitespace-changes which are partly in >> drm-misc. And I do have some upcoming additional patches for >> console support. Helge
Hi Am 11.05.23 um 19:02 schrieb Helge Deller: > On 5/11/23 16:27, Thomas Zimmermann wrote: >>>> But the work I do within fbdev is mostly for improving DRM. >>> >>> Sure. >>> >>>> For the >>>> other issues in this file, I don't think that matroxfb should even be >>>> around any longer. Fbdev has been deprecated for a long time. But a >>>> small number of drivers are still in use and we still need its >>>> framebuffer console. So someone should either put significant effort >>>> into maintaining fbdev, or it should be phased out. But neither is >>>> happening. >>> >>> You're wrong. >> > >> I'm not. I don't claim that these drivers are all broken. But fbdev >> as a whole is bit-rotting and no one attempts to address this. There >> are several recent examples of this: >> >> * I recently send out a 100-patches series to improve parameter >> parsing and avoid memory leaks. That got shot down. I didn't attempt >> to support parameter parsing for module builds. > > Your work is appreciated and it wasn't shot down, but it wasn't perfect > either. > >> * There's been a 15-yrs old bug in fbdev's read/write where they >> return an incorrect value. > > ? This one: https://patchwork.freedesktop.org/patch/534668/?series=116931&rev=2 > >> * See the other discussion on this patchset on the state of hitfb. >> >> * The fbdev code I recently cleaned up had bugs in how it uses some >> of fbdev's basic building blocks (see the screen_base/screen_buffer >> confusion). > > As you said... some (little-used/outdated) drivers may have issues > which of course show up if one starts to clean up, as you do. > On a per-driver basis it can make sense to drop a specific driver. > >> * <asm-generic/fb.h> has been in the tree since 2009 and no one >> attempted to include it until now. >> >> None of this is a sign of good maintenance. Let me add that I'm not pointing fingers at anyone. It's just the current status AFAICT. >> As I've worked on DRM's fbdev emulation a lot, I try to be a good >> kernel citizen and clean up in fbdev as well when I see a problem.> >> But I'd really like to see most of these drivers being moved into >> staging and deleted soon afterwards. Users will complain about those >> drivers that are really still required. Those might be worth to spend >> effort on. > > I'd really like to see a way forward and get the required drivers over to > DRM, e.g. based on your simpledrm driver. > If there is a way to get basic on-screen 2D bitblt and fillrect support, > it would drop the need for most of the fbdev drivers. > The current way of bitblt'ing from a buffer on regular basis istoo slow > for such older cards. Even on new hardware in emulators there > is a big slowdown visible. I'd be happy to try to drop the unused/obsolete/broken drivers. For the rest, I'd designate fbdev as a graphics console for systems without text mode. I think that was the original intention. > >>> You don't mention that for most older machines DRM isn't an >>> acceptable way to go due to it's limitations, e.g. it's low-speed >>> due to missing 2D-acceleration for older cards and and it's >>> incapability to change screen resolution at runtime (just to name >>> two of the bigger limitations here). >> >> You can change resolution at runtime; just not through fbdev ioctls. >> There's no technical limitation here. No one found any use for this, >> so it's not there. > > fbdev drivers would need that when ported to DRM. Why? Userspace would then simply use DRM ioctls to set the display mode. >>> So, unless we somehow find a good way to move such drivers over to >>> DRM (with a set of minimal 2D acceleration), they are still >>> important. >> >> 2d acceleration is mostly useful for the framebuffer console. > > and X11 > >> You can >> do that with DRM and drivers have (nouveau). It just didn't make a >> meaningful difference in most cases. > > if nouveau got it, can't it be done for simpledrm in a generic way too? Probably no, as it depends on the hardware features. The DRM driver would have to implement it's own fbdev support. That's not too complicated, but still not portable among drivers. > >>> Actually, I just did test matroxfb and pm2fb successfully a few >>> days back, and they worked. For some smaller issues I've prepared >>> patches, which are on hold due conflicts with your latest >>> file-move-around- and whitespace-changes which are partly in >>> drm-misc. And I do have some upcoming additional patches for >>> console support. > > Helge
On Thu, 11 May 2023, Thomas Zimmermann wrote: > But I'd really like to see most of these drivers being moved into > staging and deleted soon afterwards. Users will complain about those > drivers that are really still required. Those might be worth to spend > effort on. > That strategy is not going to find out what functionality is required. Instead it will find out which beneficiaries are capable of overcoming all of the hurdles to reverting deletion: - Find out how to report a regression correctly. - Gather all the necessary information. - Obtain buy-in from a sympathetic developer. - Build a patched kernel, test it and provide the results. (And possibly repeat the same until neglected code becomes accepted.) - Wait for the relevant distro to release the relevant kernel update. Developers tend to overlook the burden of process because it's ostensibly done to raise code quality. But it seems to me that affected users are more likely to seek a workaround than undertake the process. So deletion doesn't discover end-user requirements. What it does is advertise a vacancy for an unpaid adoptive maintainer, somehow presumed to be found amongst a very well remunerated and very small pool of talent. The way I look at it, the maintainence of old code is the price of a so-called "right to repair". But there ain't no free lunch and if we want that right we should seek ways to reduce that price. For example, by making a larger talent pool more effective, by re-using more code and by improving the tooling and automation. The code I'd delete first wouldn't be a small amount of old code in need of sponsorship. Or even the most buggy code. The first to go would be that code which will never find an actual end user because some portion of the code required to actually use certain platforms was never mainlined by the vendor -- and never will be without some push-back.
diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c index 9cb0685feddd..ce51227798a1 100644 --- a/drivers/video/fbdev/matrox/matroxfb_accel.c +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c @@ -88,7 +88,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { unsigned int i; - + for (i = 0; i < 16; i++) { pal[i] = i * 0x11111111U; } @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) { static inline void matrox_cfb8_pal(u_int32_t* pal) { unsigned int i; - + for (i = 0; i < 16; i++) { pal[i] = i * 0x01010101U; } @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx, /* Tell... well, why bother... */ while (height--) { size_t i; - + for (i = 0; i < step; i += 4) { /* Hope that there are at least three readable bytes beyond the end of bitmap */ fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr); diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h index 958be6805f87..c93c69bbcd57 100644 --- a/drivers/video/fbdev/matrox/matroxfb_base.h +++ b/drivers/video/fbdev/matrox/matroxfb_base.h @@ -301,9 +301,9 @@ struct matrox_altout { int (*verifymode)(void* altout_dev, u_int32_t mode); int (*getqueryctrl)(void* altout_dev, struct v4l2_queryctrl* ctrl); - int (*getctrl)(void* altout_dev, + int (*getctrl)(void *altout_dev, struct v4l2_control* ctrl); - int (*setctrl)(void* altout_dev, + int (*setctrl)(void *altout_dev, struct v4l2_control* ctrl); };