| Message ID | 20230510110557.14343-2-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand |
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); >> };
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 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); };