Re: [PATCH v2 1/3] media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Laurent,

Am Dienstag, 18. April 2023, 12:05:55 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Tue, Apr 18, 2023 at 12:00:43PM +0200, Alexander Stein wrote:
> > Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart:
> > > On Tue, Apr 18, 2023 at 09:14:37AM +0200, Alexander Stein wrote:
> > > > There is no need to convert input pixformat to mbus_framefmt and back
> > > > again. Instead apply pixformat width contrains directly.
> > > > 
> > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > 
> > > >  drivers/media/platform/nxp/imx7-media-csi.c | 14 +++++++++-----
> > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > b/drivers/media/platform/nxp/imx7-media-csi.c index
> > > > b701e823436a8..bd649fd9166fd 100644
> > > > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > > > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > > > @@ -1145,8 +1145,8 @@ static const struct imx7_csi_pixfmt *
> > > > 
> > > >  __imx7_csi_video_try_fmt(struct v4l2_pix_format *pixfmt,
> > > >  
> > > >  			 struct v4l2_rect *compose)
> > > >  
> > > >  {
> > > > 
> > > > -	struct v4l2_mbus_framefmt fmt_src;
> > > > 
> > > >  	const struct imx7_csi_pixfmt *cc;
> > > > 
> > > > +	u32 stride;
> > > > 
> > > >  	/*
> > > >  	
> > > >  	 * Find the pixel format, default to the first supported format if
> > > >  	 not
> > > > 
> > > > @@ -1172,12 +1172,16 @@ __imx7_csi_video_try_fmt(struct
> > > > v4l2_pix_format *pixfmt,> > > 
> > > >  		}
> > > >  	
> > > >  	}
> > > > 
> > > > -	v4l2_fill_mbus_format(&fmt_src, pixfmt, 0);
> > > > -	imx7_csi_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc);
> > > 
> > > Could you please keep the comment from imx7_csi_mbus_fmt_to_pix_fmt()
> > > here, to indicate where the alignment comes from ?
> > 
> > Sure, why not.
> > 
> > > 	/* Round up width for minimum burst size */
> > > 
> > > We should likely revisit this in the future, I don't think the alignment
> > > 
> > > is actually needed. This could be recorded already:
> > > 	/*
> > > 	
> > > 	 * Round up width for minimum burst size.
> > > 	 *
> > > 	 * TODO: Implement configurable stride support, and check what the 
real
> > > 	 * hardware alignment constraint on the width is.
> > > 	 */
> > 
> > Sounds good. I was already suspecting actual stride support is not
> > supported, as FBUF_PARA is set to 0 (in non-interlaced mode).
> > 
> > > > +	v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8,
> > > > +			      &pixfmt->height, 1, 0xffff, 1, 0);
> > > > +
> > > > +	stride = round_up((pixfmt->width * cc->bpp) / 8, 8);
> > > 
> > > You can drop the round_up(), as pixfmt->width is now a multiple of 8, so
> > > 
> > > 	pixfmt->bytesperline = pixfmt->width * cc->bpp / 8;
> > 
> > But that is only identical if cc->bpp == 8, or am I missing something
> > here?
> 
> If cc->bpp is equal to 16, you will get
> 
>  	pixfmt->bytesperline = pixfmt->width * 2;
> 
> which will still be a multiple of 8 as width is a multiple of 8.

That's true only for 16 bpp. If you are using e.g. 'RG10' this assumption is 
not guaranteed.  Apparently it happens to be the case for 1080p on my imx327 
sensor though.

Best regards,
Alexander

> > > > +	pixfmt->bytesperline = stride;
> > > > +	pixfmt->sizeimage = stride * pixfmt->height;
> > > > 
> > > >  	if (compose) {
> > > > 
> > > > -		compose->width = fmt_src.width;
> > > > -		compose->height = fmt_src.height;
> > > > +		compose->width = pixfmt->width;
> > > 
> > > This is a change of behaviour, compose->width used to be set to the
> > > unaligned width.
> > 
> > Oh, you are right. I'll fix that.
> > 
> > > > +		compose->height = pixfmt->height;
> > > > 
> > > >  	}
> > > >  	
> > > >  	return cc;


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/