diff mbox series

[v5,15/22] media: rzg2l-cru: Make use of v4l2_format_info() helpers

Message ID 20241011173052.1088341-16-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: platform: rzg2l-cru: CSI-2 and CRU enhancements | expand

Commit Message

Lad, Prabhakar Oct. 11, 2024, 5:30 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Make use of v4l2_format_info() helpers to determine the input and
output formats.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 22 ++++++-------------
 1 file changed, 7 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart Oct. 15, 2024, 10:33 a.m. UTC | #1 
Hi Prabhakar,

Thank you for the patch.

On Fri, Oct 11, 2024 at 06:30:45PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make use of v4l2_format_info() helpers to determine the input and
> output formats.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 22 ++++++-------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 8932fab7c656..0cc69a7440bf 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -300,21 +300,12 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  	rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
>  }
>  
> -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
>  				 const struct rzg2l_cru_ip_format *ip_fmt,
>  				 u8 csi_vc)
>  {
>  	u32 icnmc = ICnMC_INF(ip_fmt->datatype);
>  
> -	switch (ip_fmt->code) {
> -	case MEDIA_BUS_FMT_UYVY8_1X16:
> -		*input_is_yuv = true;
> -		break;
> -	default:
> -		*input_is_yuv = false;
> -		break;
> -	}
> -
>  	icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
>  
>  	/* Set virtual channel CSI2 */
> @@ -327,19 +318,17 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  					   struct v4l2_mbus_framefmt *ip_sd_fmt,
>  					   u8 csi_vc)
>  {
> +	const struct v4l2_format_info *src_finfo, *dst_finfo;
>  	const struct rzg2l_cru_ip_format *cru_ip_fmt;
> -	bool output_is_yuv = false;
> -	bool input_is_yuv = false;
>  	u32 icndmr;
>  
>  	cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> -	rzg2l_cru_csi2_setup(cru, &input_is_yuv, cru_ip_fmt, csi_vc);
> +	rzg2l_cru_csi2_setup(cru, cru_ip_fmt, csi_vc);
>  
>  	/* Output format */
>  	switch (cru->format.pixelformat) {
>  	case V4L2_PIX_FMT_UYVY:
>  		icndmr = ICnDMR_YCMODE_UYVY;
> -		output_is_yuv = true;
>  		break;
>  	default:
>  		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> @@ -347,8 +336,11 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  		return -EINVAL;
>  	}
>  
> +	src_finfo = v4l2_format_info(cru_ip_fmt->format);
> +	dst_finfo = v4l2_format_info(cru->format.pixelformat);

It would be a bit more efficient to add a yuv boolean field to the
rzg2l_cru_ip_format structure, as you already have looked up cru_ip_fmt
for the IP subdev format.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +
>  	/* If input and output use same colorspace, do bypass mode */
> -	if (output_is_yuv == input_is_yuv)
> +	if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
>  		rzg2l_cru_write(cru, ICnMC,
>  				rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
>  	else
Lad, Prabhakar Oct. 18, 2024, 11:45 a.m. UTC | #2 
Hi Laurent,

Thank you for the review.

On Tue, Oct 15, 2024 at 11:33 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Oct 11, 2024 at 06:30:45PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make use of v4l2_format_info() helpers to determine the input and
> > output formats.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 22 ++++++-------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 8932fab7c656..0cc69a7440bf 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -300,21 +300,12 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> >       rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> >  }
> >
> > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
> >                                const struct rzg2l_cru_ip_format *ip_fmt,
> >                                u8 csi_vc)
> >  {
> >       u32 icnmc = ICnMC_INF(ip_fmt->datatype);
> >
> > -     switch (ip_fmt->code) {
> > -     case MEDIA_BUS_FMT_UYVY8_1X16:
> > -             *input_is_yuv = true;
> > -             break;
> > -     default:
> > -             *input_is_yuv = false;
> > -             break;
> > -     }
> > -
> >       icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
> >
> >       /* Set virtual channel CSI2 */
> > @@ -327,19 +318,17 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> >                                          struct v4l2_mbus_framefmt *ip_sd_fmt,
> >                                          u8 csi_vc)
> >  {
> > +     const struct v4l2_format_info *src_finfo, *dst_finfo;
> >       const struct rzg2l_cru_ip_format *cru_ip_fmt;
> > -     bool output_is_yuv = false;
> > -     bool input_is_yuv = false;
> >       u32 icndmr;
> >
> >       cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> > -     rzg2l_cru_csi2_setup(cru, &input_is_yuv, cru_ip_fmt, csi_vc);
> > +     rzg2l_cru_csi2_setup(cru, cru_ip_fmt, csi_vc);
> >
> >       /* Output format */
> >       switch (cru->format.pixelformat) {
> >       case V4L2_PIX_FMT_UYVY:
> >               icndmr = ICnDMR_YCMODE_UYVY;
> > -             output_is_yuv = true;
> >               break;
> >       default:
> >               dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> > @@ -347,8 +336,11 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> >               return -EINVAL;
> >       }
> >
> > +     src_finfo = v4l2_format_info(cru_ip_fmt->format);
> > +     dst_finfo = v4l2_format_info(cru->format.pixelformat);
>
> It would be a bit more efficient to add a yuv boolean field to the
> rzg2l_cru_ip_format structure, as you already have looked up cru_ip_fmt
> for the IP subdev format.
>
I will consider this change, when adding support for the RZ/V2H SoC,
hope that's OK for you.

Cheers,
Prabhakar

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> > +
> >       /* If input and output use same colorspace, do bypass mode */
> > -     if (output_is_yuv == input_is_yuv)
> > +     if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
> >               rzg2l_cru_write(cru, ICnMC,
> >                               rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> >       else
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 18, 2024, 12:26 p.m. UTC | #3 
On Fri, Oct 18, 2024 at 12:45:36PM +0100, Lad, Prabhakar wrote:
> On Tue, Oct 15, 2024 at 11:33 AM Laurent Pinchart wrote:
> > On Fri, Oct 11, 2024 at 06:30:45PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Make use of v4l2_format_info() helpers to determine the input and
> > > output formats.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 22 ++++++-------------
> > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > index 8932fab7c656..0cc69a7440bf 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > @@ -300,21 +300,12 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> > >       rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> > >  }
> > >
> > > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> > > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
> > >                                const struct rzg2l_cru_ip_format *ip_fmt,
> > >                                u8 csi_vc)
> > >  {
> > >       u32 icnmc = ICnMC_INF(ip_fmt->datatype);
> > >
> > > -     switch (ip_fmt->code) {
> > > -     case MEDIA_BUS_FMT_UYVY8_1X16:
> > > -             *input_is_yuv = true;
> > > -             break;
> > > -     default:
> > > -             *input_is_yuv = false;
> > > -             break;
> > > -     }
> > > -
> > >       icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
> > >
> > >       /* Set virtual channel CSI2 */
> > > @@ -327,19 +318,17 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > >                                          struct v4l2_mbus_framefmt *ip_sd_fmt,
> > >                                          u8 csi_vc)
> > >  {
> > > +     const struct v4l2_format_info *src_finfo, *dst_finfo;
> > >       const struct rzg2l_cru_ip_format *cru_ip_fmt;
> > > -     bool output_is_yuv = false;
> > > -     bool input_is_yuv = false;
> > >       u32 icndmr;
> > >
> > >       cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> > > -     rzg2l_cru_csi2_setup(cru, &input_is_yuv, cru_ip_fmt, csi_vc);
> > > +     rzg2l_cru_csi2_setup(cru, cru_ip_fmt, csi_vc);
> > >
> > >       /* Output format */
> > >       switch (cru->format.pixelformat) {
> > >       case V4L2_PIX_FMT_UYVY:
> > >               icndmr = ICnDMR_YCMODE_UYVY;
> > > -             output_is_yuv = true;
> > >               break;
> > >       default:
> > >               dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> > > @@ -347,8 +336,11 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > >               return -EINVAL;
> > >       }
> > >
> > > +     src_finfo = v4l2_format_info(cru_ip_fmt->format);
> > > +     dst_finfo = v4l2_format_info(cru->format.pixelformat);
> >
> > It would be a bit more efficient to add a yuv boolean field to the
> > rzg2l_cru_ip_format structure, as you already have looked up cru_ip_fmt
> > for the IP subdev format.
>
> I will consider this change, when adding support for the RZ/V2H SoC,
> hope that's OK for you.

It's not a blocker, it can be done on top indeed. If you end up
submitting a v6 you could add a patch on top already, but otherwise
later is fine too.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > > +
> > >       /* If input and output use same colorspace, do bypass mode */
> > > -     if (output_is_yuv == input_is_yuv)
> > > +     if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
> > >               rzg2l_cru_write(cru, ICnMC,
> > >                               rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> > >       else
Lad, Prabhakar Oct. 18, 2024, 12:40 p.m. UTC | #4 
Hi Laurent,

On Fri, Oct 18, 2024 at 1:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Oct 18, 2024 at 12:45:36PM +0100, Lad, Prabhakar wrote:
> > On Tue, Oct 15, 2024 at 11:33 AM Laurent Pinchart wrote:
> > > On Fri, Oct 11, 2024 at 06:30:45PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Make use of v4l2_format_info() helpers to determine the input and
> > > > output formats.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 22 ++++++-------------
> > > >  1 file changed, 7 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > index 8932fab7c656..0cc69a7440bf 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > @@ -300,21 +300,12 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> > > >       rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> > > >  }
> > > >
> > > > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> > > > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
> > > >                                const struct rzg2l_cru_ip_format *ip_fmt,
> > > >                                u8 csi_vc)
> > > >  {
> > > >       u32 icnmc = ICnMC_INF(ip_fmt->datatype);
> > > >
> > > > -     switch (ip_fmt->code) {
> > > > -     case MEDIA_BUS_FMT_UYVY8_1X16:
> > > > -             *input_is_yuv = true;
> > > > -             break;
> > > > -     default:
> > > > -             *input_is_yuv = false;
> > > > -             break;
> > > > -     }
> > > > -
> > > >       icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
> > > >
> > > >       /* Set virtual channel CSI2 */
> > > > @@ -327,19 +318,17 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > > >                                          struct v4l2_mbus_framefmt *ip_sd_fmt,
> > > >                                          u8 csi_vc)
> > > >  {
> > > > +     const struct v4l2_format_info *src_finfo, *dst_finfo;
> > > >       const struct rzg2l_cru_ip_format *cru_ip_fmt;
> > > > -     bool output_is_yuv = false;
> > > > -     bool input_is_yuv = false;
> > > >       u32 icndmr;
> > > >
> > > >       cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> > > > -     rzg2l_cru_csi2_setup(cru, &input_is_yuv, cru_ip_fmt, csi_vc);
> > > > +     rzg2l_cru_csi2_setup(cru, cru_ip_fmt, csi_vc);
> > > >
> > > >       /* Output format */
> > > >       switch (cru->format.pixelformat) {
> > > >       case V4L2_PIX_FMT_UYVY:
> > > >               icndmr = ICnDMR_YCMODE_UYVY;
> > > > -             output_is_yuv = true;
> > > >               break;
> > > >       default:
> > > >               dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> > > > @@ -347,8 +336,11 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > +     src_finfo = v4l2_format_info(cru_ip_fmt->format);
> > > > +     dst_finfo = v4l2_format_info(cru->format.pixelformat);
> > >
> > > It would be a bit more efficient to add a yuv boolean field to the
> > > rzg2l_cru_ip_format structure, as you already have looked up cru_ip_fmt
> > > for the IP subdev format.
> >
> > I will consider this change, when adding support for the RZ/V2H SoC,
> > hope that's OK for you.
>
> It's not a blocker, it can be done on top indeed. If you end up
> submitting a v6 you could add a patch on top already, but otherwise
> later is fine too.
>
I was intending to send a v6 anyway with the updated commit messages,
so I will add a new patch on top.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 8932fab7c656..0cc69a7440bf 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -300,21 +300,12 @@  static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
 	rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
 }
 
-static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
+static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru,
 				 const struct rzg2l_cru_ip_format *ip_fmt,
 				 u8 csi_vc)
 {
 	u32 icnmc = ICnMC_INF(ip_fmt->datatype);
 
-	switch (ip_fmt->code) {
-	case MEDIA_BUS_FMT_UYVY8_1X16:
-		*input_is_yuv = true;
-		break;
-	default:
-		*input_is_yuv = false;
-		break;
-	}
-
 	icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
 
 	/* Set virtual channel CSI2 */
@@ -327,19 +318,17 @@  static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
 					   struct v4l2_mbus_framefmt *ip_sd_fmt,
 					   u8 csi_vc)
 {
+	const struct v4l2_format_info *src_finfo, *dst_finfo;
 	const struct rzg2l_cru_ip_format *cru_ip_fmt;
-	bool output_is_yuv = false;
-	bool input_is_yuv = false;
 	u32 icndmr;
 
 	cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
-	rzg2l_cru_csi2_setup(cru, &input_is_yuv, cru_ip_fmt, csi_vc);
+	rzg2l_cru_csi2_setup(cru, cru_ip_fmt, csi_vc);
 
 	/* Output format */
 	switch (cru->format.pixelformat) {
 	case V4L2_PIX_FMT_UYVY:
 		icndmr = ICnDMR_YCMODE_UYVY;
-		output_is_yuv = true;
 		break;
 	default:
 		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
@@ -347,8 +336,11 @@  static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
 		return -EINVAL;
 	}
 
+	src_finfo = v4l2_format_info(cru_ip_fmt->format);
+	dst_finfo = v4l2_format_info(cru->format.pixelformat);
+
 	/* If input and output use same colorspace, do bypass mode */
-	if (output_is_yuv == input_is_yuv)
+	if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
 		rzg2l_cru_write(cru, ICnMC,
 				rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
 	else