Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f201c5490b4c8001fd0599118aad4292.squirrel@mail.panix.com>
In-Reply-To: <26277a0c-abda-c13a-80bf-528b9e167c21@suse.de>
References: <20230419044834.10816-1-pa@panix.com>
    <26277a0c-abda-c13a-80bf-528b9e167c21@suse.de>
Date:   Wed, 19 Apr 2023 16:27:46 -0400
Subject: Re: [PATCH v2] firmware/sysfb: Fix VESA format selection
From:   "Pierre Asselin" <pa@panix.com>
To:     "Thomas Zimmermann" <tzimmermann@suse.de>
Cc:     "Pierre Asselin" <pa@panix.com>, dri-devel@lists.freedesktop.org,
        "Javier Martinez Canillas" <javierm@redhat.com>,
        "Daniel Vetter" <daniel.vetter@ffwll.ch>,
        "Ard Biesheuvel" <ardb@kernel.org>,
        "Hans de Goede" <hdegoede@redhat.com>, linux-kernel@vger.kernel.org
User-Agent: SquirrelMail/1.4.23-p1
MIME-Version: 1.0
Content-Type: text/plain;charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Importance: Normal
Precedence: bulk

Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 19.04.23 um 06:48 schrieb Pierre Asselin:
>>
>> v2 fixes the warnings from a max3() macro with arguments of different
>> types;  split the bits_per_pixel assignment to avoid uglyfing the code
>> with too many typecasts.
>
> What exactly was that warning?

A friendly note from a robot; make W=1 sysfb_simplefb.o .
https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#m38e859354329ab9f756da91e99b546e3b140fa91


> I liked the all-in-one assignment of the original patch. So I'd rather
> go back to v1 and copy si->lfb_depth to the correct type, like this:
>
>    u32 depth = si->lfb_depth;
>    bits_per_pixel = max3(max3(colors),
>    		        rsvd,
>                          depth);

Would that work?  If I understand correctly max3() checks that all args
have the same type.  {red,green,blue,rsvd}.{size,pos} are all u8 while
lfb_depth is u16.  The best I can do is

    bits_per_pixel = max3((u16)max3(si->red_size + si->red_pos,
                                    si->green_size + si->green_pos,
                                    si->blue_size + si->blue_pos),
                          (u16)(si->rsvd_size + si->rsvd_pos),
                          si->lfb_depth);

That compiles quietly with W=1 but those two casts are ugly.
If I do that, would K&R-on-parentheses read better ?

    bits_per_pixel = max3(
                          (u16)max3(
                                  si->red_size + si->red_pos,
                                  si->green_size + si->green_pos,
                                  si->blue_size + si->blue_pos
                          ),
                          (u16)(si->rsvd_size + si->rsvd_pos),
                          si->lfb_depth
                     );

I think it's clearer, but not kernel style and still ugly.

> Or, if you want to get fancy, you could add max3_t() to <linux/minmax.h>
>
>    #define max3_t(type, x, y, z)   max_t(type, max_t(type, x, y), z)
>
> and do
>
>    bits_per_pixel = max3_t(u32,
>                            max3(colors),
>                            rsvd,
>                            si->lfb_depth)
>
> You could also add a max4_t(type, x, y, z, w) to <linux/minmax.h> and
> compare all values with max4_t().

That would be a two-patch series.  I'd rather keep it to the strict
minimum that fixes the regression.  (You trust me to even *look* at a
kernel header and not break it ?  Dangerous assumption!)

I'm new at this.  Two months ago I didn't know what to type a the
command line after "git".

Incidentally, should I send v3 as a new email or reply to the chain?

--PA