[PATCH] video: Add parentheses around VNBYTES() macro

[PATCH] video: Add parentheses around VNBYTES() macro

[Dan Carpenter]

The VNBYTES() macro needs to have parentheses to prevent some (harmless)
macro expansion bugs.  The VNBYTES() macro is used like this:

	VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)

The * operation is done before the / operation.  It still ends up with
the same results, but it's not ideal.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 include/video.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/video.h b/include/video.h
index 9729fa348aa5..b364bf91825f 100644
--- a/include/video.h
+++ b/include/video.h
@@ -58,7 +58,7 @@ enum video_log2_bpp {
  * Convert enum video_log2_bpp to bytes and bits. Note we omit the outer
  * brackets to allow multiplication by fractional pixels.
  */
-#define VNBYTES(bpix)	(1 << (bpix)) / 8
+#define VNBYTES(bpix)	((1 << (bpix)) / 8)
 
 #define VNBITS(bpix)	(1 << (bpix))
 
-- 
2.39.2

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Simon Glass]

On Wed, 26 Jul 2023 at 00:54, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> macro expansion bugs.  The VNBYTES() macro is used like this:
>
>         VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
>
> The * operation is done before the / operation.  It still ends up with
> the same results, but it's not ideal.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  include/video.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'd argue that doing * before / is better, since it might be more
accurate. But I don't know if it actually matters.

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:

> The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> macro expansion bugs.  The VNBYTES() macro is used like this:
> 
> 	VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> 
> The * operation is done before the / operation.  It still ends up with
> the same results, but it's not ideal.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

In that this seems correct:

Applied to u-boot/next, thanks!

But I want to note that with gcc-13.1 (and binutils 2.40) or more
specifically the kernel.org arm/aarch64/etc toolchains, this causes the
generated code to change:
   aarch64: (for 1/1 boards) all +8.0 text +8.0
            bananapi-m5    : all +8 text +8
               u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
                 function                                   old     new   delta
                 video_post_probe                           248     256      +8

So I don't know that this was a harmless bug.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Simon Glass]

Hi Tom, Dan,

On Tue, 8 Aug 2023 at 19:39, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
>
> > The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> > macro expansion bugs.  The VNBYTES() macro is used like this:
> >
> >       VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> >
> > The * operation is done before the / operation.  It still ends up with
> > the same results, but it's not ideal.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> In that this seems correct:
>
> Applied to u-boot/next, thanks!
>
> But I want to note that with gcc-13.1 (and binutils 2.40) or more
> specifically the kernel.org arm/aarch64/etc toolchains, this causes the
> generated code to change:
>    aarch64: (for 1/1 boards) all +8.0 text +8.0
>             bananapi-m5    : all +8 text +8
>                u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
>                  function                                   old     new   delta
>                  video_post_probe                           248     256      +8
>
> So I don't know that this was a harmless bug.

Hmm I should have read the comment directly above...

"Note we omit the outer brackets to allow multiplication by fractional pixels."

Having said that, it doesn't seem to matter getting (if indeed we do)
a less accurate result.

Regards,
Simon

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
> Hi Tom, Dan,
> 
> On Tue, 8 Aug 2023 at 19:39, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
> >
> > > The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> > > macro expansion bugs.  The VNBYTES() macro is used like this:
> > >
> > >       VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> > >
> > > The * operation is done before the / operation.  It still ends up with
> > > the same results, but it's not ideal.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > In that this seems correct:
> >
> > Applied to u-boot/next, thanks!
> >
> > But I want to note that with gcc-13.1 (and binutils 2.40) or more
> > specifically the kernel.org arm/aarch64/etc toolchains, this causes the
> > generated code to change:
> >    aarch64: (for 1/1 boards) all +8.0 text +8.0
> >             bananapi-m5    : all +8 text +8
> >                u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
> >                  function                                   old     new   delta
> >                  video_post_probe                           248     256      +8
> >
> > So I don't know that this was a harmless bug.
> 
> Hmm I should have read the comment directly above...
> 
> "Note we omit the outer brackets to allow multiplication by fractional pixels."
> 
> Having said that, it doesn't seem to matter getting (if indeed we do)
> a less accurate result.

But we should fix the comment then, or decide no, this is intentional.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Miquel Raynal]

Hello,

trini@konsulko.com wrote on Tue, 19 Sep 2023 11:14:56 -0400:

> On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
> > Hi Tom, Dan,
> > 
> > On Tue, 8 Aug 2023 at 19:39, Tom Rini <trini@konsulko.com> wrote:  
> > >
> > > On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
> > >  
> > > > The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> > > > macro expansion bugs.  The VNBYTES() macro is used like this:
> > > >
> > > >       VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> > > >
> > > > The * operation is done before the / operation.  It still ends up with
> > > > the same results, but it's not ideal.
> > > >
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>  
> > >
> > > In that this seems correct:
> > >
> > > Applied to u-boot/next, thanks!
> > >
> > > But I want to note that with gcc-13.1 (and binutils 2.40) or more
> > > specifically the kernel.org arm/aarch64/etc toolchains, this causes the
> > > generated code to change:
> > >    aarch64: (for 1/1 boards) all +8.0 text +8.0
> > >             bananapi-m5    : all +8 text +8
> > >                u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
> > >                  function                                   old     new   delta
> > >                  video_post_probe                           248     256      +8
> > >
> > > So I don't know that this was a harmless bug.  
> > 
> > Hmm I should have read the comment directly above...
> > 
> > "Note we omit the outer brackets to allow multiplication by fractional pixels."
> > 
> > Having said that, it doesn't seem to matter getting (if indeed we do)
> > a less accurate result.  
> 
> But we should fix the comment then, or decide no, this is intentional.

One year has passed and I just stumbled upon this incoherency. It would
be nice to clarify the comment or otherwise to revert Dan's patch if
judged irrelevant in the end.

Thanks!
Miquèl

Re: [PATCH] video: Add parentheses around VNBYTES() macro

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

On Mon, Sep 02, 2024 at 10:17:59AM +0200, Miquel Raynal wrote:
> Hello,
> 
> trini@konsulko.com wrote on Tue, 19 Sep 2023 11:14:56 -0400:
> 
> > On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
> > > Hi Tom, Dan,
> > > 
> > > On Tue, 8 Aug 2023 at 19:39, Tom Rini <trini@konsulko.com> wrote:  
> > > >
> > > > On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
> > > >  
> > > > > The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> > > > > macro expansion bugs.  The VNBYTES() macro is used like this:
> > > > >
> > > > >       VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> > > > >
> > > > > The * operation is done before the / operation.  It still ends up with
> > > > > the same results, but it's not ideal.
> > > > >
> > > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > > Reviewed-by: Simon Glass <sjg@chromium.org>  
> > > >
> > > > In that this seems correct:
> > > >
> > > > Applied to u-boot/next, thanks!
> > > >
> > > > But I want to note that with gcc-13.1 (and binutils 2.40) or more
> > > > specifically the kernel.org arm/aarch64/etc toolchains, this causes the
> > > > generated code to change:
> > > >    aarch64: (for 1/1 boards) all +8.0 text +8.0
> > > >             bananapi-m5    : all +8 text +8
> > > >                u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
> > > >                  function                                   old     new   delta
> > > >                  video_post_probe                           248     256      +8
> > > >
> > > > So I don't know that this was a harmless bug.  
> > > 
> > > Hmm I should have read the comment directly above...
> > > 
> > > "Note we omit the outer brackets to allow multiplication by fractional pixels."
> > > 
> > > Having said that, it doesn't seem to matter getting (if indeed we do)
> > > a less accurate result.  
> > 
> > But we should fix the comment then, or decide no, this is intentional.
> 
> One year has passed and I just stumbled upon this incoherency. It would
> be nice to clarify the comment or otherwise to revert Dan's patch if
> judged irrelevant in the end.

I guess at this point I vote for fixing the comment.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]