Message-ID: <e7bd021c-1a6b-6e47-143a-36ae2fd2fe6b@suse.de>
Date:   Thu, 11 May 2023 16:27:12 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Helge Deller <deller@....de>, Sui Jingfeng <15330273260@....cn>,
        geert@...ux-m68k.org, javierm@...hat.com, daniel@...ll.ch,
        vgupta@...nel.org, chenhuacai@...nel.org, kernel@...0n.name,
        davem@...emloft.net, arnd@...db.de, sam@...nborg.org
Cc:     linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-arch@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-ia64@...r.kernel.org,
        loongarch@...ts.linux.dev, linux-m68k@...ts.linux-m68k.org,
        sparclinux@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-parisc@...r.kernel.org
Subject: Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

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@...e.de>
>>>> Reviewed-by: Arnd Bergmann <arnd@...db.de>
>>>> Reviewed-by: Sam Ravnborg <sam@...nborg.org>
>>>> Reviewed-by: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>> Tested-by: Sui Jingfeng <suijingfeng@...ngson.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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists