| Message ID | 20230418071439.197735-2-alexander.stein@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] media: imx: imx7-media-csi: Get rid of superfluous call to imx7_csi_mbus_fmt_to_pix_fmt | expand |
Hi Laurent, thanks or your feedback. Am Dienstag, 18. April 2023, 11:27:13 CEST schrieb Laurent Pinchart: > Hi Alexander, > > Thank you for the patch. > > 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? > > + 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. Best regards, Alexander > > + compose->height = pixfmt->height; > > > > } > > > > return cc;
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. > > > + 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;
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); + v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8, + &pixfmt->height, 1, 0xffff, 1, 0); + + stride = round_up((pixfmt->width * cc->bpp) / 8, 8); + pixfmt->bytesperline = stride; + pixfmt->sizeimage = stride * pixfmt->height; if (compose) { - compose->width = fmt_src.width; - compose->height = fmt_src.height; + compose->width = pixfmt->width; + compose->height = pixfmt->height; } return cc;
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(-)