| Message ID | 20230418071439.197735-2-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix imx7-media-csi format settings | expand |
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 ? /* 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. */ > + 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; > + 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. > + compose->height = pixfmt->height; > } > > return cc;
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;
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;
Hi Alexander, On Tue, Apr 18, 2023 at 12:08:07PM +0200, Alexander Stein wrote: > Am Dienstag, 18. April 2023, 12:05:55 CEST schrieb Laurent Pinchart: > > 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. For 10-bit bayer formats, cc->bpp is 16. The only bpp values used by the driver are 8 and 16. > > > > > + 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(-)