[PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Hans Verkuil]

The CENTERED_RECTANGLE define fails to compile on clang and old gcc
versions. Just drop it and fill in the crop rectangles explicitly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 6428eb5394a9..8490618c5071 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -407,14 +407,6 @@ static const struct imx283_reg_list link_freq_reglist[] = {
 	},
 };

-#define CENTERED_RECTANGLE(rect, _width, _height)			\
-	{								\
-		.left = rect.left + ((rect.width - (_width)) / 2),	\
-		.top = rect.top + ((rect.height - (_height)) / 2),	\
-		.width = (_width),					\
-		.height = (_height),					\
-	}
-
 /* Mode configs */
 static const struct imx283_mode supported_modes_12bit[] = {
 	{
@@ -440,7 +432,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
 		.min_shr = 11,
 		.horizontal_ob = 96,
 		.vertical_ob = 16,
-		.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
+		.crop = {
+			.top = 40,
+			.left = 108,
+			.width = 5472,
+			.height = 3648,
+		},
 	},
 	{
 		/*
@@ -468,7 +465,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
 		.horizontal_ob = 48,
 		.vertical_ob = 4,

-		.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
+		.crop = {
+			.top = 40,
+			.left = 108,
+			.width = 5472,
+			.height = 3648,
+		},
 	},
 };

@@ -489,7 +491,12 @@ static const struct imx283_mode supported_modes_10bit[] = {
 		.min_shr = 10,
 		.horizontal_ob = 96,
 		.vertical_ob = 16,
-		.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
+		.crop = {
+			.top = 40,
+			.left = 108,
+			.width = 5472,
+			.height = 3648,
+		},
 	},
 };

Re: [PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Kieran Bingham]

Quoting Hans Verkuil (2024-06-13 16:19:08)
> The CENTERED_RECTANGLE define fails to compile on clang and old gcc
> versions. Just drop it and fill in the crop rectangles explicitly.

So back when I was playing around with this I thought it would have got
dropped during review / upstreaming before now - because I couldn't find
a way to make sure the macro args were guaranteed to be used only once,
by putting some locals in the macro (because of the initialisation).

So I'm not surprised that it needs to be removed, but I am surprised
that it wasn't for the reason I expected ;-)

Anyway - maybe later I'll experiement with more common helpers perhaps -
but not if it hits compile errors..



> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 6428eb5394a9..8490618c5071 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -407,14 +407,6 @@ static const struct imx283_reg_list link_freq_reglist[] = {
>         },
>  };
> 
> -#define CENTERED_RECTANGLE(rect, _width, _height)                      \
> -       {                                                               \
> -               .left = rect.left + ((rect.width - (_width)) / 2),      \
> -               .top = rect.top + ((rect.height - (_height)) / 2),      \
> -               .width = (_width),                                      \
> -               .height = (_height),                                    \
> -       }
> -
>  /* Mode configs */
>  static const struct imx283_mode supported_modes_12bit[] = {
>         {
> @@ -440,7 +432,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
>                 .min_shr = 11,
>                 .horizontal_ob = 96,
>                 .vertical_ob = 16,
> -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> +               .crop = {
> +                       .top = 40,
> +                       .left = 108,
> +                       .width = 5472,
> +                       .height = 3648,
> +               },

I do recall using v4l2_rects to define the active area so they could be
used collectively rather than initialising things as
	.width = WIDTH,
	.height = HEIGHT,

So - perhaps this could be (if it compiles):
	.crop = imx283_active_area,

but either way is fine with me if it unbreaks linux-next.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         },
>         {
>                 /*
> @@ -468,7 +465,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
>                 .horizontal_ob = 48,
>                 .vertical_ob = 4,
> 
> -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> +               .crop = {
> +                       .top = 40,
> +                       .left = 108,
> +                       .width = 5472,
> +                       .height = 3648,
> +               },
>         },
>  };
> 
> @@ -489,7 +491,12 @@ static const struct imx283_mode supported_modes_10bit[] = {
>                 .min_shr = 10,
>                 .horizontal_ob = 96,
>                 .vertical_ob = 16,
> -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> +               .crop = {
> +                       .top = 40,
> +                       .left = 108,
> +                       .width = 5472,
> +                       .height = 3648,
> +               },
>         },
>  };
> 
>

Re: [PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Sakari Ailus]

Hi Kieran, Hans,

Thanks for working on this.

On Thu, Jun 13, 2024 at 04:26:43PM +0100, Kieran Bingham wrote:
> Quoting Hans Verkuil (2024-06-13 16:19:08)
> > The CENTERED_RECTANGLE define fails to compile on clang and old gcc
> > versions. Just drop it and fill in the crop rectangles explicitly.
> 
> So back when I was playing around with this I thought it would have got
> dropped during review / upstreaming before now - because I couldn't find
> a way to make sure the macro args were guaranteed to be used only once,
> by putting some locals in the macro (because of the initialisation).
> 
> So I'm not surprised that it needs to be removed, but I am surprised
> that it wasn't for the reason I expected ;-)
> 
> Anyway - maybe later I'll experiement with more common helpers perhaps -
> but not if it hits compile errors..

Or once clang before ~ 17 is deprecated? :-)

> 
> 
> 
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> > index 6428eb5394a9..8490618c5071 100644
> > --- a/drivers/media/i2c/imx283.c
> > +++ b/drivers/media/i2c/imx283.c
> > @@ -407,14 +407,6 @@ static const struct imx283_reg_list link_freq_reglist[] = {
> >         },
> >  };
> > 
> > -#define CENTERED_RECTANGLE(rect, _width, _height)                      \
> > -       {                                                               \
> > -               .left = rect.left + ((rect.width - (_width)) / 2),      \
> > -               .top = rect.top + ((rect.height - (_height)) / 2),      \
> > -               .width = (_width),                                      \
> > -               .height = (_height),                                    \
> > -       }
> > -
> >  /* Mode configs */
> >  static const struct imx283_mode supported_modes_12bit[] = {
> >         {
> > @@ -440,7 +432,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
> >                 .min_shr = 11,
> >                 .horizontal_ob = 96,
> >                 .vertical_ob = 16,
> > -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> > +               .crop = {
> > +                       .top = 40,
> > +                       .left = 108,
> > +                       .width = 5472,
> > +                       .height = 3648,
> > +               },
> 
> I do recall using v4l2_rects to define the active area so they could be
> used collectively rather than initialising things as
> 	.width = WIDTH,
> 	.height = HEIGHT,

I'd prefer this, too. Plain numbers are easier to get wrong.

> 
> So - perhaps this could be (if it compiles):
> 	.crop = imx283_active_area,

This is my preference as well.

> 
> but either way is fine with me if it unbreaks linux-next.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Either way, as a clang compilation workaround this is ok so:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> >         },
> >         {
> >                 /*
> > @@ -468,7 +465,12 @@ static const struct imx283_mode supported_modes_12bit[] = {
> >                 .horizontal_ob = 48,
> >                 .vertical_ob = 4,
> > 
> > -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> > +               .crop = {
> > +                       .top = 40,
> > +                       .left = 108,
> > +                       .width = 5472,
> > +                       .height = 3648,
> > +               },
> >         },
> >  };
> > 
> > @@ -489,7 +491,12 @@ static const struct imx283_mode supported_modes_10bit[] = {
> >                 .min_shr = 10,
> >                 .horizontal_ob = 96,
> >                 .vertical_ob = 16,
> > -               .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> > +               .crop = {
> > +                       .top = 40,
> > +                       .left = 108,
> > +                       .width = 5472,
> > +                       .height = 3648,
> > +               },
> >         },
> >  };
> > 
> >

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Mauro Carvalho Chehab]

Em Thu, 13 Jun 2024 15:43:04 +0000
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Kieran, Hans,
> 
> Thanks for working on this.
> 
> On Thu, Jun 13, 2024 at 04:26:43PM +0100, Kieran Bingham wrote:
> > Quoting Hans Verkuil (2024-06-13 16:19:08)  
> > > The CENTERED_RECTANGLE define fails to compile on clang and old gcc
> > > versions. Just drop it and fill in the crop rectangles explicitly.  
> > 
> > So back when I was playing around with this I thought it would have got
> > dropped during review / upstreaming before now - because I couldn't find
> > a way to make sure the macro args were guaranteed to be used only once,
> > by putting some locals in the macro (because of the initialisation).
> > 
> > So I'm not surprised that it needs to be removed, but I am surprised
> > that it wasn't for the reason I expected ;-)
> > 
> > Anyway - maybe later I'll experiement with more common helpers perhaps -
> > but not if it hits compile errors..  
> 
> Or once clang before ~ 17 is deprecated? :-)


It is not deprecated for Kernel builds. See Documentation/process/changes.rst:

<snip>
Current Minimal Requirements
****************************

Upgrade to at **least** these software revisions before thinking you've
encountered a bug!  If you're unsure what version you're currently
running, the suggested command should tell you.

Again, keep in mind that this list assumes you are already functionally
running a Linux kernel.  Also, not all tools are necessary on all
systems; obviously, if you don't have any PC Card hardware, for example,
you probably needn't concern yourself with pcmciautils.

====================== ===============  ========================================
        Program        Minimal version       Command to check the version
====================== ===============  ========================================
GNU C                  5.1              gcc --version
Clang/LLVM (optional)  13.0.1           clang --version
</snip>

Regards,
Mauro


Thanks,
Mauro

Re: [PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Mauro Carvalho Chehab]

Em Thu, 13 Jun 2024 16:26:43 +0100
Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu:

> Quoting Hans Verkuil (2024-06-13 16:19:08)
> > The CENTERED_RECTANGLE define fails to compile on clang and old gcc
> > versions. Just drop it and fill in the crop rectangles explicitly.  
> 
> So back when I was playing around with this I thought it would have got
> dropped during review / upstreaming before now - because I couldn't find
> a way to make sure the macro args were guaranteed to be used only once,
> by putting some locals in the macro (because of the initialisation).
> 
> So I'm not surprised that it needs to be removed, but I am surprised
> that it wasn't for the reason I expected ;-)
> 
> Anyway - maybe later I'll experiement with more common helpers perhaps -
> but not if it hits compile errors..

IMO, a helper just makes it worse for humans. I mean, just looking at:

	.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),

I can't tell what values for top/left would be used.

> I do recall using v4l2_rects to define the active area so they could be
> used collectively rather than initialising things as
> 	.width = WIDTH,
> 	.height = HEIGHT,

Using defines for the minimum/maximum visible area makes sense,
e. g. something similar to this:

               .crop = {
                      .top = MIN_VISIBLE_TOP,
                      .left = MIN_VISIBLE_LEFT,
                      .width = MAX_WIDTH,
                      .height = MAX_HEIGHT,
               },

would also be fine, as it would be clear that the crop region is
the one containing the hardware limits.

> So - perhaps this could be (if it compiles):
>	.crop = imx283_active_area,

This should equally work, but maybe you could do, instead:

	.crop = &imx283_active_area,	// e.g. using a pointer

to avoid duplicating for every supported mode.

Thanks,
Mauro