* [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-05-30 7:49 ` Ming Qian
0 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-05-30 7:49 UTC (permalink / raw)
To: mchehab, mirela.rabulea, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
The hardware can support any image size WxH,
with arbitrary W (image width) and H (image height) dimensions.
Align upwards buffer size for both encoder and decoder.
and leave the picture resolution unchanged.
For decoder, the risk of memory out of bounds can be avoided.
For both encoder and decoder, the driver will lift the limitation of
resolution alignment.
For example, the decoder can support jpeg whose resolution is 227x149
the encoder can support nv12 1080P, won't change it to 1920x1072.
Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v2
- add Fixes tag
.../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
1 file changed, 37 insertions(+), 51 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index c0fd030d0f19..9a1c8df522ed 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
jpeg->slot_data[slot].cfg_stream_size =
mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
q_data->fmt->fourcc,
- q_data->w_adjusted,
- q_data->h_adjusted);
+ q_data->w,
+ q_data->h);
/* chain the config descriptor with the encoding descriptor */
cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
@@ -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
&q_data_cap->h_adjusted,
q_data_cap->h_adjusted, /* adjust up */
MXC_JPEG_MAX_HEIGHT,
- q_data_cap->fmt->v_align,
+ 0,
0);
/* setup bytesperline/sizeimage for capture queue */
@@ -1161,18 +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
{
struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
struct mxc_jpeg_q_data *q_data = NULL;
+ struct mxc_jpeg_q_data tmp_q;
int i;
q_data = mxc_jpeg_get_q_data(ctx, q->type);
if (!q_data)
return -EINVAL;
+ tmp_q.fmt = q_data->fmt;
+ tmp_q.w = q_data->w_adjusted;
+ tmp_q.h = q_data->h_adjusted;
+ for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
+ tmp_q.bytesperline[i] = q_data->bytesperline[i];
+ tmp_q.sizeimage[i] = q_data->sizeimage[i];
+ }
+ mxc_jpeg_sizeimage(&tmp_q);
+ for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
+ tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
+
/* Handle CREATE_BUFS situation - *nplanes != 0 */
if (*nplanes) {
if (*nplanes != q_data->fmt->colplanes)
return -EINVAL;
for (i = 0; i < *nplanes; i++) {
- if (sizes[i] < q_data->sizeimage[i])
+ if (sizes[i] < tmp_q.sizeimage[i])
return -EINVAL;
}
return 0;
@@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
/* Handle REQBUFS situation */
*nplanes = q_data->fmt->colplanes;
for (i = 0; i < *nplanes; i++)
- sizes[i] = q_data->sizeimage[i];
+ sizes[i] = tmp_q.sizeimage[i];
return 0;
}
@@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
}
q_data_out->w = header.frame.width;
q_data_out->h = header.frame.height;
- if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
- dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
- header.frame.width, header.frame.height);
- return -EINVAL;
- }
if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
header.frame.height > MXC_JPEG_MAX_HEIGHT) {
dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
@@ -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fm
pix_mp->num_planes = fmt->colplanes;
pix_mp->pixelformat = fmt->fourcc;
- /*
- * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
- * alignment, to loosen up the alignment to multiple of 8,
- * otherwise NV12-1080p fails as 1080 is not a multiple of 16
- */
+ pix_mp->width = w;
+ pix_mp->height = h;
v4l_bound_align_image(&w,
- MXC_JPEG_MIN_WIDTH,
- w, /* adjust downwards*/
+ w, /* adjust upwards*/
+ MXC_JPEG_MAX_WIDTH,
fmt->h_align,
&h,
- MXC_JPEG_MIN_HEIGHT,
- h, /* adjust downwards*/
- MXC_JPEG_H_ALIGN,
+ h, /* adjust upwards*/
+ MXC_JPEG_MAX_HEIGHT,
+ 0,
0);
- pix_mp->width = w; /* negotiate the width */
- pix_mp->height = h; /* negotiate the height */
/* get user input into the tmp_q */
tmp_q.w = w;
@@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx *ctx,
q_data->w_adjusted = q_data->w;
q_data->h_adjusted = q_data->h;
- if (jpeg->mode == MXC_JPEG_DECODE) {
- /*
- * align up the resolution for CAST IP,
- * but leave the buffer resolution unchanged
- */
- v4l_bound_align_image(&q_data->w_adjusted,
- q_data->w_adjusted, /* adjust upwards */
- MXC_JPEG_MAX_WIDTH,
- q_data->fmt->h_align,
- &q_data->h_adjusted,
- q_data->h_adjusted, /* adjust upwards */
- MXC_JPEG_MAX_HEIGHT,
- q_data->fmt->v_align,
- 0);
- } else {
- /*
- * align down the resolution for CAST IP,
- * but leave the buffer resolution unchanged
- */
- v4l_bound_align_image(&q_data->w_adjusted,
- MXC_JPEG_MIN_WIDTH,
- q_data->w_adjusted, /* adjust downwards*/
- q_data->fmt->h_align,
- &q_data->h_adjusted,
- MXC_JPEG_MIN_HEIGHT,
- q_data->h_adjusted, /* adjust downwards*/
- q_data->fmt->v_align,
- 0);
- }
+ /*
+ * align up the resolution for CAST IP,
+ * but leave the buffer resolution unchanged
+ */
+ v4l_bound_align_image(&q_data->w_adjusted,
+ q_data->w_adjusted, /* adjust upwards */
+ MXC_JPEG_MAX_WIDTH,
+ q_data->fmt->h_align,
+ &q_data->h_adjusted,
+ q_data->h_adjusted, /* adjust upwards */
+ MXC_JPEG_MAX_HEIGHT,
+ q_data->fmt->v_align,
+ 0);
for (i = 0; i < pix_mp->num_planes; i++) {
q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
--
2.36.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-05-30 7:49 ` Ming Qian
0 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-05-30 7:49 UTC (permalink / raw)
To: mchehab, mirela.rabulea, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
The hardware can support any image size WxH,
with arbitrary W (image width) and H (image height) dimensions.
Align upwards buffer size for both encoder and decoder.
and leave the picture resolution unchanged.
For decoder, the risk of memory out of bounds can be avoided.
For both encoder and decoder, the driver will lift the limitation of
resolution alignment.
For example, the decoder can support jpeg whose resolution is 227x149
the encoder can support nv12 1080P, won't change it to 1920x1072.
Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v2
- add Fixes tag
.../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
1 file changed, 37 insertions(+), 51 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index c0fd030d0f19..9a1c8df522ed 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
jpeg->slot_data[slot].cfg_stream_size =
mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
q_data->fmt->fourcc,
- q_data->w_adjusted,
- q_data->h_adjusted);
+ q_data->w,
+ q_data->h);
/* chain the config descriptor with the encoding descriptor */
cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
@@ -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
&q_data_cap->h_adjusted,
q_data_cap->h_adjusted, /* adjust up */
MXC_JPEG_MAX_HEIGHT,
- q_data_cap->fmt->v_align,
+ 0,
0);
/* setup bytesperline/sizeimage for capture queue */
@@ -1161,18 +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
{
struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
struct mxc_jpeg_q_data *q_data = NULL;
+ struct mxc_jpeg_q_data tmp_q;
int i;
q_data = mxc_jpeg_get_q_data(ctx, q->type);
if (!q_data)
return -EINVAL;
+ tmp_q.fmt = q_data->fmt;
+ tmp_q.w = q_data->w_adjusted;
+ tmp_q.h = q_data->h_adjusted;
+ for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
+ tmp_q.bytesperline[i] = q_data->bytesperline[i];
+ tmp_q.sizeimage[i] = q_data->sizeimage[i];
+ }
+ mxc_jpeg_sizeimage(&tmp_q);
+ for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
+ tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
+
/* Handle CREATE_BUFS situation - *nplanes != 0 */
if (*nplanes) {
if (*nplanes != q_data->fmt->colplanes)
return -EINVAL;
for (i = 0; i < *nplanes; i++) {
- if (sizes[i] < q_data->sizeimage[i])
+ if (sizes[i] < tmp_q.sizeimage[i])
return -EINVAL;
}
return 0;
@@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
/* Handle REQBUFS situation */
*nplanes = q_data->fmt->colplanes;
for (i = 0; i < *nplanes; i++)
- sizes[i] = q_data->sizeimage[i];
+ sizes[i] = tmp_q.sizeimage[i];
return 0;
}
@@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
}
q_data_out->w = header.frame.width;
q_data_out->h = header.frame.height;
- if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
- dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
- header.frame.width, header.frame.height);
- return -EINVAL;
- }
if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
header.frame.height > MXC_JPEG_MAX_HEIGHT) {
dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
@@ -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fm
pix_mp->num_planes = fmt->colplanes;
pix_mp->pixelformat = fmt->fourcc;
- /*
- * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
- * alignment, to loosen up the alignment to multiple of 8,
- * otherwise NV12-1080p fails as 1080 is not a multiple of 16
- */
+ pix_mp->width = w;
+ pix_mp->height = h;
v4l_bound_align_image(&w,
- MXC_JPEG_MIN_WIDTH,
- w, /* adjust downwards*/
+ w, /* adjust upwards*/
+ MXC_JPEG_MAX_WIDTH,
fmt->h_align,
&h,
- MXC_JPEG_MIN_HEIGHT,
- h, /* adjust downwards*/
- MXC_JPEG_H_ALIGN,
+ h, /* adjust upwards*/
+ MXC_JPEG_MAX_HEIGHT,
+ 0,
0);
- pix_mp->width = w; /* negotiate the width */
- pix_mp->height = h; /* negotiate the height */
/* get user input into the tmp_q */
tmp_q.w = w;
@@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx *ctx,
q_data->w_adjusted = q_data->w;
q_data->h_adjusted = q_data->h;
- if (jpeg->mode == MXC_JPEG_DECODE) {
- /*
- * align up the resolution for CAST IP,
- * but leave the buffer resolution unchanged
- */
- v4l_bound_align_image(&q_data->w_adjusted,
- q_data->w_adjusted, /* adjust upwards */
- MXC_JPEG_MAX_WIDTH,
- q_data->fmt->h_align,
- &q_data->h_adjusted,
- q_data->h_adjusted, /* adjust upwards */
- MXC_JPEG_MAX_HEIGHT,
- q_data->fmt->v_align,
- 0);
- } else {
- /*
- * align down the resolution for CAST IP,
- * but leave the buffer resolution unchanged
- */
- v4l_bound_align_image(&q_data->w_adjusted,
- MXC_JPEG_MIN_WIDTH,
- q_data->w_adjusted, /* adjust downwards*/
- q_data->fmt->h_align,
- &q_data->h_adjusted,
- MXC_JPEG_MIN_HEIGHT,
- q_data->h_adjusted, /* adjust downwards*/
- q_data->fmt->v_align,
- 0);
- }
+ /*
+ * align up the resolution for CAST IP,
+ * but leave the buffer resolution unchanged
+ */
+ v4l_bound_align_image(&q_data->w_adjusted,
+ q_data->w_adjusted, /* adjust upwards */
+ MXC_JPEG_MAX_WIDTH,
+ q_data->fmt->h_align,
+ &q_data->h_adjusted,
+ q_data->h_adjusted, /* adjust upwards */
+ MXC_JPEG_MAX_HEIGHT,
+ q_data->fmt->v_align,
+ 0);
for (i = 0; i < pix_mp->num_planes; i++) {
q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
--
2.36.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
2022-05-30 7:49 ` Ming Qian
@ 2022-06-12 22:55 ` mirela.rabulea
-1 siblings, 0 replies; 10+ messages in thread
From: mirela.rabulea @ 2022-06-12 22:55 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
Hi Ming,
On 30.05.2022 10:49, Ming Qian wrote:
> The hardware can support any image size WxH,
> with arbitrary W (image width) and H (image height) dimensions.
>
> Align upwards buffer size for both encoder and decoder.
> and leave the picture resolution unchanged.
>
> For decoder, the risk of memory out of bounds can be avoided.
> For both encoder and decoder, the driver will lift the limitation of
> resolution alignment.
>
> For example, the decoder can support jpeg whose resolution is 227x149
I doubt 227x149 is working. I have tried 127x127, with your patch
applied, both on encoder and decoder, the image does not look ok. The
126x127 seems to work. Having an odd resolution seems strange, I see not
even gstreamer supports it (tried videotestsrc & filesink with BGR,
127x128 produces a 128x128 size).
We need to gain more clarity on this one.
And when we do, if we really can support any arbitrary resolution, from
both the jpeg core and wrapper point of view, we have stuff to clean up.
The assumption that I started with was, as stated in the comments:
* The alignment requirements for the resolution depend on the format,
* multiple of 16 resolutions should work for all formats.
* Special workarounds are made in the driver to support NV12 1080p.
With h_align/v_align defined in mxc_formats[].
Regards,
Mirela
> the encoder can support nv12 1080P, won't change it to 1920x1072.
>
> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v2
> - add Fixes tag
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
> 1 file changed, 37 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index c0fd030d0f19..9a1c8df522ed 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
> jpeg->slot_data[slot].cfg_stream_size =
> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> q_data->fmt->fourcc,
> - q_data->w_adjusted,
> - q_data->h_adjusted);
> + q_data->w,
> + q_data->h);
>
> /* chain the config descriptor with the encoding descriptor */
> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
> @@ -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
> &q_data_cap->h_adjusted,
> q_data_cap->h_adjusted, /* adjust up */
> MXC_JPEG_MAX_HEIGHT,
> - q_data_cap->fmt->v_align,
> + 0,
> 0);
>
> /* setup bytesperline/sizeimage for capture queue */
> @@ -1161,18 +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> {
> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> struct mxc_jpeg_q_data *q_data = NULL;
> + struct mxc_jpeg_q_data tmp_q;
> int i;
>
> q_data = mxc_jpeg_get_q_data(ctx, q->type);
> if (!q_data)
> return -EINVAL;
>
> + tmp_q.fmt = q_data->fmt;
> + tmp_q.w = q_data->w_adjusted;
> + tmp_q.h = q_data->h_adjusted;
> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> + }
> + mxc_jpeg_sizeimage(&tmp_q);
> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
> +
> /* Handle CREATE_BUFS situation - *nplanes != 0 */
> if (*nplanes) {
> if (*nplanes != q_data->fmt->colplanes)
> return -EINVAL;
> for (i = 0; i < *nplanes; i++) {
> - if (sizes[i] < q_data->sizeimage[i])
> + if (sizes[i] < tmp_q.sizeimage[i])
> return -EINVAL;
> }
> return 0;
> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> /* Handle REQBUFS situation */
> *nplanes = q_data->fmt->colplanes;
> for (i = 0; i < *nplanes; i++)
> - sizes[i] = q_data->sizeimage[i];
> + sizes[i] = tmp_q.sizeimage[i];
>
> return 0;
> }
> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
> }
> q_data_out->w = header.frame.width;
> q_data_out->h = header.frame.height;
> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> - header.frame.width, header.frame.height);
> - return -EINVAL;
> - }
> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
> @@ -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fm
> pix_mp->num_planes = fmt->colplanes;
> pix_mp->pixelformat = fmt->fourcc;
>
> - /*
> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> - * alignment, to loosen up the alignment to multiple of 8,
> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> - */
> + pix_mp->width = w;
> + pix_mp->height = h;
> v4l_bound_align_image(&w,
> - MXC_JPEG_MIN_WIDTH,
> - w, /* adjust downwards*/
> + w, /* adjust upwards*/
> + MXC_JPEG_MAX_WIDTH,
> fmt->h_align,
> &h,
> - MXC_JPEG_MIN_HEIGHT,
> - h, /* adjust downwards*/
> - MXC_JPEG_H_ALIGN,
> + h, /* adjust upwards*/
> + MXC_JPEG_MAX_HEIGHT,
> + 0,
> 0);
> - pix_mp->width = w; /* negotiate the width */
> - pix_mp->height = h; /* negotiate the height */
>
> /* get user input into the tmp_q */
> tmp_q.w = w;
> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx *ctx,
>
> q_data->w_adjusted = q_data->w;
> q_data->h_adjusted = q_data->h;
> - if (jpeg->mode == MXC_JPEG_DECODE) {
> - /*
> - * align up the resolution for CAST IP,
> - * but leave the buffer resolution unchanged
> - */
> - v4l_bound_align_image(&q_data->w_adjusted,
> - q_data->w_adjusted, /* adjust upwards */
> - MXC_JPEG_MAX_WIDTH,
> - q_data->fmt->h_align,
> - &q_data->h_adjusted,
> - q_data->h_adjusted, /* adjust upwards */
> - MXC_JPEG_MAX_HEIGHT,
> - q_data->fmt->v_align,
> - 0);
> - } else {
> - /*
> - * align down the resolution for CAST IP,
> - * but leave the buffer resolution unchanged
> - */
> - v4l_bound_align_image(&q_data->w_adjusted,
> - MXC_JPEG_MIN_WIDTH,
> - q_data->w_adjusted, /* adjust downwards*/
> - q_data->fmt->h_align,
> - &q_data->h_adjusted,
> - MXC_JPEG_MIN_HEIGHT,
> - q_data->h_adjusted, /* adjust downwards*/
> - q_data->fmt->v_align,
> - 0);
> - }
> + /*
> + * align up the resolution for CAST IP,
> + * but leave the buffer resolution unchanged
> + */
> + v4l_bound_align_image(&q_data->w_adjusted,
> + q_data->w_adjusted, /* adjust upwards */
> + MXC_JPEG_MAX_WIDTH,
> + q_data->fmt->h_align,
> + &q_data->h_adjusted,
> + q_data->h_adjusted, /* adjust upwards */
> + MXC_JPEG_MAX_HEIGHT,
> + q_data->fmt->v_align,
> + 0);
>
> for (i = 0; i < pix_mp->num_planes; i++) {
> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-06-12 22:55 ` mirela.rabulea
0 siblings, 0 replies; 10+ messages in thread
From: mirela.rabulea @ 2022-06-12 22:55 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
Hi Ming,
On 30.05.2022 10:49, Ming Qian wrote:
> The hardware can support any image size WxH,
> with arbitrary W (image width) and H (image height) dimensions.
>
> Align upwards buffer size for both encoder and decoder.
> and leave the picture resolution unchanged.
>
> For decoder, the risk of memory out of bounds can be avoided.
> For both encoder and decoder, the driver will lift the limitation of
> resolution alignment.
>
> For example, the decoder can support jpeg whose resolution is 227x149
I doubt 227x149 is working. I have tried 127x127, with your patch
applied, both on encoder and decoder, the image does not look ok. The
126x127 seems to work. Having an odd resolution seems strange, I see not
even gstreamer supports it (tried videotestsrc & filesink with BGR,
127x128 produces a 128x128 size).
We need to gain more clarity on this one.
And when we do, if we really can support any arbitrary resolution, from
both the jpeg core and wrapper point of view, we have stuff to clean up.
The assumption that I started with was, as stated in the comments:
* The alignment requirements for the resolution depend on the format,
* multiple of 16 resolutions should work for all formats.
* Special workarounds are made in the driver to support NV12 1080p.
With h_align/v_align defined in mxc_formats[].
Regards,
Mirela
> the encoder can support nv12 1080P, won't change it to 1920x1072.
>
> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v2
> - add Fixes tag
> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
> 1 file changed, 37 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index c0fd030d0f19..9a1c8df522ed 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct vb2_buffer *out_buf,
> jpeg->slot_data[slot].cfg_stream_size =
> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> q_data->fmt->fourcc,
> - q_data->w_adjusted,
> - q_data->h_adjusted);
> + q_data->w,
> + q_data->h);
>
> /* chain the config descriptor with the encoding descriptor */
> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
> @@ -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
> &q_data_cap->h_adjusted,
> q_data_cap->h_adjusted, /* adjust up */
> MXC_JPEG_MAX_HEIGHT,
> - q_data_cap->fmt->v_align,
> + 0,
> 0);
>
> /* setup bytesperline/sizeimage for capture queue */
> @@ -1161,18 +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> {
> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> struct mxc_jpeg_q_data *q_data = NULL;
> + struct mxc_jpeg_q_data tmp_q;
> int i;
>
> q_data = mxc_jpeg_get_q_data(ctx, q->type);
> if (!q_data)
> return -EINVAL;
>
> + tmp_q.fmt = q_data->fmt;
> + tmp_q.w = q_data->w_adjusted;
> + tmp_q.h = q_data->h_adjusted;
> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> + }
> + mxc_jpeg_sizeimage(&tmp_q);
> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
> +
> /* Handle CREATE_BUFS situation - *nplanes != 0 */
> if (*nplanes) {
> if (*nplanes != q_data->fmt->colplanes)
> return -EINVAL;
> for (i = 0; i < *nplanes; i++) {
> - if (sizes[i] < q_data->sizeimage[i])
> + if (sizes[i] < tmp_q.sizeimage[i])
> return -EINVAL;
> }
> return 0;
> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> /* Handle REQBUFS situation */
> *nplanes = q_data->fmt->colplanes;
> for (i = 0; i < *nplanes; i++)
> - sizes[i] = q_data->sizeimage[i];
> + sizes[i] = tmp_q.sizeimage[i];
>
> return 0;
> }
> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
> }
> q_data_out->w = header.frame.width;
> q_data_out->h = header.frame.height;
> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> - header.frame.width, header.frame.height);
> - return -EINVAL;
> - }
> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
> @@ -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f, const struct mxc_jpeg_fmt *fm
> pix_mp->num_planes = fmt->colplanes;
> pix_mp->pixelformat = fmt->fourcc;
>
> - /*
> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> - * alignment, to loosen up the alignment to multiple of 8,
> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> - */
> + pix_mp->width = w;
> + pix_mp->height = h;
> v4l_bound_align_image(&w,
> - MXC_JPEG_MIN_WIDTH,
> - w, /* adjust downwards*/
> + w, /* adjust upwards*/
> + MXC_JPEG_MAX_WIDTH,
> fmt->h_align,
> &h,
> - MXC_JPEG_MIN_HEIGHT,
> - h, /* adjust downwards*/
> - MXC_JPEG_H_ALIGN,
> + h, /* adjust upwards*/
> + MXC_JPEG_MAX_HEIGHT,
> + 0,
> 0);
> - pix_mp->width = w; /* negotiate the width */
> - pix_mp->height = h; /* negotiate the height */
>
> /* get user input into the tmp_q */
> tmp_q.w = w;
> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx *ctx,
>
> q_data->w_adjusted = q_data->w;
> q_data->h_adjusted = q_data->h;
> - if (jpeg->mode == MXC_JPEG_DECODE) {
> - /*
> - * align up the resolution for CAST IP,
> - * but leave the buffer resolution unchanged
> - */
> - v4l_bound_align_image(&q_data->w_adjusted,
> - q_data->w_adjusted, /* adjust upwards */
> - MXC_JPEG_MAX_WIDTH,
> - q_data->fmt->h_align,
> - &q_data->h_adjusted,
> - q_data->h_adjusted, /* adjust upwards */
> - MXC_JPEG_MAX_HEIGHT,
> - q_data->fmt->v_align,
> - 0);
> - } else {
> - /*
> - * align down the resolution for CAST IP,
> - * but leave the buffer resolution unchanged
> - */
> - v4l_bound_align_image(&q_data->w_adjusted,
> - MXC_JPEG_MIN_WIDTH,
> - q_data->w_adjusted, /* adjust downwards*/
> - q_data->fmt->h_align,
> - &q_data->h_adjusted,
> - MXC_JPEG_MIN_HEIGHT,
> - q_data->h_adjusted, /* adjust downwards*/
> - q_data->fmt->v_align,
> - 0);
> - }
> + /*
> + * align up the resolution for CAST IP,
> + * but leave the buffer resolution unchanged
> + */
> + v4l_bound_align_image(&q_data->w_adjusted,
> + q_data->w_adjusted, /* adjust upwards */
> + MXC_JPEG_MAX_WIDTH,
> + q_data->fmt->h_align,
> + &q_data->h_adjusted,
> + q_data->h_adjusted, /* adjust upwards */
> + MXC_JPEG_MAX_HEIGHT,
> + q_data->fmt->v_align,
> + 0);
>
> for (i = 0; i < pix_mp->num_planes; i++) {
> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2] media: imx-jpeg: Align upwards buffer size
2022-06-12 22:55 ` mirela.rabulea
@ 2022-06-13 5:25 ` Ming Qian
-1 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-06-13 5:25 UTC (permalink / raw)
To: Mirela Rabulea (OSS), mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> Sent: 2022年6月13日 6:56
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> hverkuil-cisco@xs4all.nl
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>
> Hi Ming,
>
> On 30.05.2022 10:49, Ming Qian wrote:
> > The hardware can support any image size WxH, with arbitrary W (image
> > width) and H (image height) dimensions.
> >
> > Align upwards buffer size for both encoder and decoder.
> > and leave the picture resolution unchanged.
> >
> > For decoder, the risk of memory out of bounds can be avoided.
> > For both encoder and decoder, the driver will lift the limitation of
> > resolution alignment.
> >
> > For example, the decoder can support jpeg whose resolution is 227x149
>
> I doubt 227x149 is working. I have tried 127x127, with your patch applied,
> both on encoder and decoder, the image does not look ok. The
> 126x127 seems to work. Having an odd resolution seems strange, I see not
> even gstreamer supports it (tried videotestsrc & filesink with BGR,
> 127x128 produces a 128x128 size).
>
> We need to gain more clarity on this one.
> And when we do, if we really can support any arbitrary resolution, from both
> the jpeg core and wrapper point of view, we have stuff to clean up.
> The assumption that I started with was, as stated in the comments:
> * The alignment requirements for the resolution depend on the format,
> * multiple of 16 resolutions should work for all formats.
> * Special workarounds are made in the driver to support NV12 1080p.
> With h_align/v_align defined in mxc_formats[].
>
> Regards,
> Mirela
Hi Mirela,
I think you are confusing picture size and buffer size.
In this patch, driver will enlarge the buffer size to align 16x16. But let the picture size unchanged.
And if you display the decoded 227x149 picture directly on imx8q, you may meet some drm error, and the display looks abnormal,
The error message like below:
[ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
[ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen requests to read empty FIFO
[ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
But if you save the decoded picture data in to a file, then check the data, you will find it's correct.
And if you treat the decoded buffer as a picture with resolution 240x160, and with some padding content, it's also correct.
So in my first patch, I let the driver report the aligned resolution in g_fmt,
and implement a g_selection to report the actual resolution through the crop info.
but this solution will fail your labgrid jpeg test.
So I choose the current solution that keep the actual picture size, and align upwards the buffer size.
The display of 227x149 is abnormal, I think it's not the jpeg codec's limitation, but the imx8q drm's limitation.
And in my opinion, I prefer the first solution that implementing a g_selection to report the actual picture size.
If you can accept that changing your labgrid jpeg testcase, I can prepare a v3 patch to switch to this solution.
Ming
>
> > the encoder can support nv12 1080P, won't change it to 1920x1072.
> >
> > Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG
> > Encoder/Decoder")
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> > v2
> > - add Fixes tag
> > .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
> > 1 file changed, 37 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > index c0fd030d0f19..9a1c8df522ed 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
> vb2_buffer *out_buf,
> > jpeg->slot_data[slot].cfg_stream_size =
> > mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> > q_data->fmt->fourcc,
> > - q_data->w_adjusted,
> > - q_data->h_adjusted);
> > + q_data->w,
> > + q_data->h);
> >
> > /* chain the config descriptor with the encoding descriptor */
> > cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; @@
> > -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx
> *ctx,
> > &q_data_cap->h_adjusted,
> > q_data_cap->h_adjusted, /* adjust up */
> > MXC_JPEG_MAX_HEIGHT,
> > - q_data_cap->fmt->v_align,
> > + 0,
> > 0);
> >
> > /* setup bytesperline/sizeimage for capture queue */ @@ -1161,18
> > +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> > {
> > struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > struct mxc_jpeg_q_data *q_data = NULL;
> > + struct mxc_jpeg_q_data tmp_q;
> > int i;
> >
> > q_data = mxc_jpeg_get_q_data(ctx, q->type);
> > if (!q_data)
> > return -EINVAL;
> >
> > + tmp_q.fmt = q_data->fmt;
> > + tmp_q.w = q_data->w_adjusted;
> > + tmp_q.h = q_data->h_adjusted;
> > + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> > + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> > + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> > + }
> > + mxc_jpeg_sizeimage(&tmp_q);
> > + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> > + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
> > +
> > /* Handle CREATE_BUFS situation - *nplanes != 0 */
> > if (*nplanes) {
> > if (*nplanes != q_data->fmt->colplanes)
> > return -EINVAL;
> > for (i = 0; i < *nplanes; i++) {
> > - if (sizes[i] < q_data->sizeimage[i])
> > + if (sizes[i] < tmp_q.sizeimage[i])
> > return -EINVAL;
> > }
> > return 0;
> > @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
> vb2_queue *q,
> > /* Handle REQBUFS situation */
> > *nplanes = q_data->fmt->colplanes;
> > for (i = 0; i < *nplanes; i++)
> > - sizes[i] = q_data->sizeimage[i];
> > + sizes[i] = tmp_q.sizeimage[i];
> >
> > return 0;
> > }
> > @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
> *ctx, struct vb2_buffer *vb)
> > }
> > q_data_out->w = header.frame.width;
> > q_data_out->h = header.frame.height;
> > - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> > - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> > - header.frame.width, header.frame.height);
> > - return -EINVAL;
> > - }
> > if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> > header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> > dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
> @@
> > -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f,
> const struct mxc_jpeg_fmt *fm
> > pix_mp->num_planes = fmt->colplanes;
> > pix_mp->pixelformat = fmt->fourcc;
> >
> > - /*
> > - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> > - * alignment, to loosen up the alignment to multiple of 8,
> > - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> > - */
> > + pix_mp->width = w;
> > + pix_mp->height = h;
> > v4l_bound_align_image(&w,
> > - MXC_JPEG_MIN_WIDTH,
> > - w, /* adjust downwards*/
> > + w, /* adjust upwards*/
> > + MXC_JPEG_MAX_WIDTH,
> > fmt->h_align,
> > &h,
> > - MXC_JPEG_MIN_HEIGHT,
> > - h, /* adjust downwards*/
> > - MXC_JPEG_H_ALIGN,
> > + h, /* adjust upwards*/
> > + MXC_JPEG_MAX_HEIGHT,
> > + 0,
> > 0);
> > - pix_mp->width = w; /* negotiate the width */
> > - pix_mp->height = h; /* negotiate the height */
> >
> > /* get user input into the tmp_q */
> > tmp_q.w = w;
> > @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx
> > *ctx,
> >
> > q_data->w_adjusted = q_data->w;
> > q_data->h_adjusted = q_data->h;
> > - if (jpeg->mode == MXC_JPEG_DECODE) {
> > - /*
> > - * align up the resolution for CAST IP,
> > - * but leave the buffer resolution unchanged
> > - */
> > - v4l_bound_align_image(&q_data->w_adjusted,
> > - q_data->w_adjusted, /* adjust upwards */
> > - MXC_JPEG_MAX_WIDTH,
> > - q_data->fmt->h_align,
> > - &q_data->h_adjusted,
> > - q_data->h_adjusted, /* adjust upwards */
> > - MXC_JPEG_MAX_HEIGHT,
> > - q_data->fmt->v_align,
> > - 0);
> > - } else {
> > - /*
> > - * align down the resolution for CAST IP,
> > - * but leave the buffer resolution unchanged
> > - */
> > - v4l_bound_align_image(&q_data->w_adjusted,
> > - MXC_JPEG_MIN_WIDTH,
> > - q_data->w_adjusted, /* adjust downwards*/
> > - q_data->fmt->h_align,
> > - &q_data->h_adjusted,
> > - MXC_JPEG_MIN_HEIGHT,
> > - q_data->h_adjusted, /* adjust downwards*/
> > - q_data->fmt->v_align,
> > - 0);
> > - }
> > + /*
> > + * align up the resolution for CAST IP,
> > + * but leave the buffer resolution unchanged
> > + */
> > + v4l_bound_align_image(&q_data->w_adjusted,
> > + q_data->w_adjusted, /* adjust upwards */
> > + MXC_JPEG_MAX_WIDTH,
> > + q_data->fmt->h_align,
> > + &q_data->h_adjusted,
> > + q_data->h_adjusted, /* adjust upwards */
> > + MXC_JPEG_MAX_HEIGHT,
> > + q_data->fmt->v_align,
> > + 0);
> >
> > for (i = 0; i < pix_mp->num_planes; i++) {
> > q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-06-13 5:25 ` Ming Qian
0 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-06-13 5:25 UTC (permalink / raw)
To: Mirela Rabulea (OSS), mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> Sent: 2022年6月13日 6:56
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> hverkuil-cisco@xs4all.nl
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>
> Hi Ming,
>
> On 30.05.2022 10:49, Ming Qian wrote:
> > The hardware can support any image size WxH, with arbitrary W (image
> > width) and H (image height) dimensions.
> >
> > Align upwards buffer size for both encoder and decoder.
> > and leave the picture resolution unchanged.
> >
> > For decoder, the risk of memory out of bounds can be avoided.
> > For both encoder and decoder, the driver will lift the limitation of
> > resolution alignment.
> >
> > For example, the decoder can support jpeg whose resolution is 227x149
>
> I doubt 227x149 is working. I have tried 127x127, with your patch applied,
> both on encoder and decoder, the image does not look ok. The
> 126x127 seems to work. Having an odd resolution seems strange, I see not
> even gstreamer supports it (tried videotestsrc & filesink with BGR,
> 127x128 produces a 128x128 size).
>
> We need to gain more clarity on this one.
> And when we do, if we really can support any arbitrary resolution, from both
> the jpeg core and wrapper point of view, we have stuff to clean up.
> The assumption that I started with was, as stated in the comments:
> * The alignment requirements for the resolution depend on the format,
> * multiple of 16 resolutions should work for all formats.
> * Special workarounds are made in the driver to support NV12 1080p.
> With h_align/v_align defined in mxc_formats[].
>
> Regards,
> Mirela
Hi Mirela,
I think you are confusing picture size and buffer size.
In this patch, driver will enlarge the buffer size to align 16x16. But let the picture size unchanged.
And if you display the decoded 227x149 picture directly on imx8q, you may meet some drm error, and the display looks abnormal,
The error message like below:
[ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
[ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen requests to read empty FIFO
[ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
But if you save the decoded picture data in to a file, then check the data, you will find it's correct.
And if you treat the decoded buffer as a picture with resolution 240x160, and with some padding content, it's also correct.
So in my first patch, I let the driver report the aligned resolution in g_fmt,
and implement a g_selection to report the actual resolution through the crop info.
but this solution will fail your labgrid jpeg test.
So I choose the current solution that keep the actual picture size, and align upwards the buffer size.
The display of 227x149 is abnormal, I think it's not the jpeg codec's limitation, but the imx8q drm's limitation.
And in my opinion, I prefer the first solution that implementing a g_selection to report the actual picture size.
If you can accept that changing your labgrid jpeg testcase, I can prepare a v3 patch to switch to this solution.
Ming
>
> > the encoder can support nv12 1080P, won't change it to 1920x1072.
> >
> > Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG
> > Encoder/Decoder")
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> > v2
> > - add Fixes tag
> > .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
> > 1 file changed, 37 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > index c0fd030d0f19..9a1c8df522ed 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
> vb2_buffer *out_buf,
> > jpeg->slot_data[slot].cfg_stream_size =
> > mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> > q_data->fmt->fourcc,
> > - q_data->w_adjusted,
> > - q_data->h_adjusted);
> > + q_data->w,
> > + q_data->h);
> >
> > /* chain the config descriptor with the encoding descriptor */
> > cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; @@
> > -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx
> *ctx,
> > &q_data_cap->h_adjusted,
> > q_data_cap->h_adjusted, /* adjust up */
> > MXC_JPEG_MAX_HEIGHT,
> > - q_data_cap->fmt->v_align,
> > + 0,
> > 0);
> >
> > /* setup bytesperline/sizeimage for capture queue */ @@ -1161,18
> > +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> > {
> > struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > struct mxc_jpeg_q_data *q_data = NULL;
> > + struct mxc_jpeg_q_data tmp_q;
> > int i;
> >
> > q_data = mxc_jpeg_get_q_data(ctx, q->type);
> > if (!q_data)
> > return -EINVAL;
> >
> > + tmp_q.fmt = q_data->fmt;
> > + tmp_q.w = q_data->w_adjusted;
> > + tmp_q.h = q_data->h_adjusted;
> > + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> > + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> > + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> > + }
> > + mxc_jpeg_sizeimage(&tmp_q);
> > + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> > + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
> > +
> > /* Handle CREATE_BUFS situation - *nplanes != 0 */
> > if (*nplanes) {
> > if (*nplanes != q_data->fmt->colplanes)
> > return -EINVAL;
> > for (i = 0; i < *nplanes; i++) {
> > - if (sizes[i] < q_data->sizeimage[i])
> > + if (sizes[i] < tmp_q.sizeimage[i])
> > return -EINVAL;
> > }
> > return 0;
> > @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
> vb2_queue *q,
> > /* Handle REQBUFS situation */
> > *nplanes = q_data->fmt->colplanes;
> > for (i = 0; i < *nplanes; i++)
> > - sizes[i] = q_data->sizeimage[i];
> > + sizes[i] = tmp_q.sizeimage[i];
> >
> > return 0;
> > }
> > @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
> *ctx, struct vb2_buffer *vb)
> > }
> > q_data_out->w = header.frame.width;
> > q_data_out->h = header.frame.height;
> > - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> > - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> > - header.frame.width, header.frame.height);
> > - return -EINVAL;
> > - }
> > if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> > header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> > dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
> @@
> > -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f,
> const struct mxc_jpeg_fmt *fm
> > pix_mp->num_planes = fmt->colplanes;
> > pix_mp->pixelformat = fmt->fourcc;
> >
> > - /*
> > - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> > - * alignment, to loosen up the alignment to multiple of 8,
> > - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> > - */
> > + pix_mp->width = w;
> > + pix_mp->height = h;
> > v4l_bound_align_image(&w,
> > - MXC_JPEG_MIN_WIDTH,
> > - w, /* adjust downwards*/
> > + w, /* adjust upwards*/
> > + MXC_JPEG_MAX_WIDTH,
> > fmt->h_align,
> > &h,
> > - MXC_JPEG_MIN_HEIGHT,
> > - h, /* adjust downwards*/
> > - MXC_JPEG_H_ALIGN,
> > + h, /* adjust upwards*/
> > + MXC_JPEG_MAX_HEIGHT,
> > + 0,
> > 0);
> > - pix_mp->width = w; /* negotiate the width */
> > - pix_mp->height = h; /* negotiate the height */
> >
> > /* get user input into the tmp_q */
> > tmp_q.w = w;
> > @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx
> > *ctx,
> >
> > q_data->w_adjusted = q_data->w;
> > q_data->h_adjusted = q_data->h;
> > - if (jpeg->mode == MXC_JPEG_DECODE) {
> > - /*
> > - * align up the resolution for CAST IP,
> > - * but leave the buffer resolution unchanged
> > - */
> > - v4l_bound_align_image(&q_data->w_adjusted,
> > - q_data->w_adjusted, /* adjust upwards */
> > - MXC_JPEG_MAX_WIDTH,
> > - q_data->fmt->h_align,
> > - &q_data->h_adjusted,
> > - q_data->h_adjusted, /* adjust upwards */
> > - MXC_JPEG_MAX_HEIGHT,
> > - q_data->fmt->v_align,
> > - 0);
> > - } else {
> > - /*
> > - * align down the resolution for CAST IP,
> > - * but leave the buffer resolution unchanged
> > - */
> > - v4l_bound_align_image(&q_data->w_adjusted,
> > - MXC_JPEG_MIN_WIDTH,
> > - q_data->w_adjusted, /* adjust downwards*/
> > - q_data->fmt->h_align,
> > - &q_data->h_adjusted,
> > - MXC_JPEG_MIN_HEIGHT,
> > - q_data->h_adjusted, /* adjust downwards*/
> > - q_data->fmt->v_align,
> > - 0);
> > - }
> > + /*
> > + * align up the resolution for CAST IP,
> > + * but leave the buffer resolution unchanged
> > + */
> > + v4l_bound_align_image(&q_data->w_adjusted,
> > + q_data->w_adjusted, /* adjust upwards */
> > + MXC_JPEG_MAX_WIDTH,
> > + q_data->fmt->h_align,
> > + &q_data->h_adjusted,
> > + q_data->h_adjusted, /* adjust upwards */
> > + MXC_JPEG_MAX_HEIGHT,
> > + q_data->fmt->v_align,
> > + 0);
> >
> > for (i = 0; i < pix_mp->num_planes; i++) {
> > q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
2022-06-13 5:25 ` Ming Qian
@ 2022-06-16 7:30 ` mirela.rabulea
-1 siblings, 0 replies; 10+ messages in thread
From: mirela.rabulea @ 2022-06-16 7:30 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
Hi Ming,
On 13.06.2022 08:25, Ming Qian wrote:
>> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
>> Sent: 2022年6月13日 6:56
>> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
>> hverkuil-cisco@xs4all.nl
>> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
>> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>>
>> Hi Ming,
>>
>> On 30.05.2022 10:49, Ming Qian wrote:
>>> The hardware can support any image size WxH, with arbitrary W (image
>>> width) and H (image height) dimensions.
>>>
>>> Align upwards buffer size for both encoder and decoder.
>>> and leave the picture resolution unchanged.
>>>
>>> For decoder, the risk of memory out of bounds can be avoided.
>>> For both encoder and decoder, the driver will lift the limitation of
>>> resolution alignment.
>>>
>>> For example, the decoder can support jpeg whose resolution is 227x149
>>
>> I doubt 227x149 is working. I have tried 127x127, with your patch applied,
>> both on encoder and decoder, the image does not look ok. The
>> 126x127 seems to work. Having an odd resolution seems strange, I see not
>> even gstreamer supports it (tried videotestsrc & filesink with BGR,
>> 127x128 produces a 128x128 size).
>>
>> We need to gain more clarity on this one.
>> And when we do, if we really can support any arbitrary resolution, from both
>> the jpeg core and wrapper point of view, we have stuff to clean up.
>> The assumption that I started with was, as stated in the comments:
>> * The alignment requirements for the resolution depend on the format,
>> * multiple of 16 resolutions should work for all formats.
>> * Special workarounds are made in the driver to support NV12 1080p.
>> With h_align/v_align defined in mxc_formats[].
>>
>> Regards,
>> Mirela
>
> Hi Mirela,
> I think you are confusing picture size and buffer size.
> In this patch, driver will enlarge the buffer size to align 16x16. But let the picture size unchanged.
> And if you display the decoded 227x149 picture directly on imx8q, you may meet some drm error, and the display looks abnormal,
> The error message like below:
> [ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
> [ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen requests to read empty FIFO
> [ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
>
> But if you save the decoded picture data in to a file, then check the data, you will find it's correct.
> And if you treat the decoded buffer as a picture with resolution 240x160, and with some padding content, it's also correct.
>
> So in my first patch, I let the driver report the aligned resolution in g_fmt,
> and implement a g_selection to report the actual resolution through the crop info.
> but this solution will fail your labgrid jpeg test.
>
> So I choose the current solution that keep the actual picture size, and align upwards the buffer size.
>
> The display of 227x149 is abnormal, I think it's not the jpeg codec's limitation, but the imx8q drm's limitation.
I did not try to display with gstreamer. I just tried to generate some
test files. For example, this one:
gst-launch-1.0 videotestsrc pattern=smpte75 num-buffers=1 !
video/x-raw,width=227,height=149,format=BGR ! filesink
location=bgr_227x149.raw
generates a 228x149 file, not 227x149, with the 228 column black
(padding?). For visualizing, I used vooya, on host machine.
I then tried the pattern generator: https://github.com/NXPmicro/nxp-patgen
./patgen.exe -pix_fmt yuv444 -p colorbar -vsize 227x149
This generates a raw yuv444 227x149, without padding.
I used the unit test application to encode it.
The resulting jpeg is reported by JPEGSnoop to have Image Size =
227x149, and it looks bad, every line is shifted, as if it would have
228 width.
I did not check yet to see if any adjustments are needed in the unit test.
I'll send you my test files.
Regards,
Mirela
>
> And in my opinion, I prefer the first solution that implementing a g_selection to report the actual picture size.
> If you can accept that changing your labgrid jpeg testcase, I can prepare a v3 patch to switch to this solution.
>
> Ming
>
>>
>>> the encoder can support nv12 1080P, won't change it to 1920x1072.
>>>
>>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG
>>> Encoder/Decoder")
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v2
>>> - add Fixes tag
>>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
>>> 1 file changed, 37 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> index c0fd030d0f19..9a1c8df522ed 100644
>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
>> vb2_buffer *out_buf,
>>> jpeg->slot_data[slot].cfg_stream_size =
>>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>>> q_data->fmt->fourcc,
>>> - q_data->w_adjusted,
>>> - q_data->h_adjusted);
>>> + q_data->w,
>>> + q_data->h);
>>>
>>> /* chain the config descriptor with the encoding descriptor */
>>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; @@
>>> -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx
>> *ctx,
>>> &q_data_cap->h_adjusted,
>>> q_data_cap->h_adjusted, /* adjust up */
>>> MXC_JPEG_MAX_HEIGHT,
>>> - q_data_cap->fmt->v_align,
>>> + 0,
>>> 0);
>>>
>>> /* setup bytesperline/sizeimage for capture queue */ @@ -1161,18
>>> +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
>>> {
>>> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
>>> struct mxc_jpeg_q_data *q_data = NULL;
>>> + struct mxc_jpeg_q_data tmp_q;
>>> int i;
>>>
>>> q_data = mxc_jpeg_get_q_data(ctx, q->type);
>>> if (!q_data)
>>> return -EINVAL;
>>>
>>> + tmp_q.fmt = q_data->fmt;
>>> + tmp_q.w = q_data->w_adjusted;
>>> + tmp_q.h = q_data->h_adjusted;
>>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
>>> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
>>> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
>>> + }
>>> + mxc_jpeg_sizeimage(&tmp_q);
>>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
>>> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
>>> +
>>> /* Handle CREATE_BUFS situation - *nplanes != 0 */
>>> if (*nplanes) {
>>> if (*nplanes != q_data->fmt->colplanes)
>>> return -EINVAL;
>>> for (i = 0; i < *nplanes; i++) {
>>> - if (sizes[i] < q_data->sizeimage[i])
>>> + if (sizes[i] < tmp_q.sizeimage[i])
>>> return -EINVAL;
>>> }
>>> return 0;
>>> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
>> vb2_queue *q,
>>> /* Handle REQBUFS situation */
>>> *nplanes = q_data->fmt->colplanes;
>>> for (i = 0; i < *nplanes; i++)
>>> - sizes[i] = q_data->sizeimage[i];
>>> + sizes[i] = tmp_q.sizeimage[i];
>>>
>>> return 0;
>>> }
>>> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
>> *ctx, struct vb2_buffer *vb)
>>> }
>>> q_data_out->w = header.frame.width;
>>> q_data_out->h = header.frame.height;
>>> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
>>> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
>>> - header.frame.width, header.frame.height);
>>> - return -EINVAL;
>>> - }
>>> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
>>> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
>>> dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
>> @@
>>> -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f,
>> const struct mxc_jpeg_fmt *fm
>>> pix_mp->num_planes = fmt->colplanes;
>>> pix_mp->pixelformat = fmt->fourcc;
>>>
>>> - /*
>>> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
>>> - * alignment, to loosen up the alignment to multiple of 8,
>>> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
>>> - */
>>> + pix_mp->width = w;
>>> + pix_mp->height = h;
>>> v4l_bound_align_image(&w,
>>> - MXC_JPEG_MIN_WIDTH,
>>> - w, /* adjust downwards*/
>>> + w, /* adjust upwards*/
>>> + MXC_JPEG_MAX_WIDTH,
>>> fmt->h_align,
>>> &h,
>>> - MXC_JPEG_MIN_HEIGHT,
>>> - h, /* adjust downwards*/
>>> - MXC_JPEG_H_ALIGN,
>>> + h, /* adjust upwards*/
>>> + MXC_JPEG_MAX_HEIGHT,
>>> + 0,
>>> 0);
>>> - pix_mp->width = w; /* negotiate the width */
>>> - pix_mp->height = h; /* negotiate the height */
>>>
>>> /* get user input into the tmp_q */
>>> tmp_q.w = w;
>>> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx
>>> *ctx,
>>>
>>> q_data->w_adjusted = q_data->w;
>>> q_data->h_adjusted = q_data->h;
>>> - if (jpeg->mode == MXC_JPEG_DECODE) {
>>> - /*
>>> - * align up the resolution for CAST IP,
>>> - * but leave the buffer resolution unchanged
>>> - */
>>> - v4l_bound_align_image(&q_data->w_adjusted,
>>> - q_data->w_adjusted, /* adjust upwards */
>>> - MXC_JPEG_MAX_WIDTH,
>>> - q_data->fmt->h_align,
>>> - &q_data->h_adjusted,
>>> - q_data->h_adjusted, /* adjust upwards */
>>> - MXC_JPEG_MAX_HEIGHT,
>>> - q_data->fmt->v_align,
>>> - 0);
>>> - } else {
>>> - /*
>>> - * align down the resolution for CAST IP,
>>> - * but leave the buffer resolution unchanged
>>> - */
>>> - v4l_bound_align_image(&q_data->w_adjusted,
>>> - MXC_JPEG_MIN_WIDTH,
>>> - q_data->w_adjusted, /* adjust downwards*/
>>> - q_data->fmt->h_align,
>>> - &q_data->h_adjusted,
>>> - MXC_JPEG_MIN_HEIGHT,
>>> - q_data->h_adjusted, /* adjust downwards*/
>>> - q_data->fmt->v_align,
>>> - 0);
>>> - }
>>> + /*
>>> + * align up the resolution for CAST IP,
>>> + * but leave the buffer resolution unchanged
>>> + */
>>> + v4l_bound_align_image(&q_data->w_adjusted,
>>> + q_data->w_adjusted, /* adjust upwards */
>>> + MXC_JPEG_MAX_WIDTH,
>>> + q_data->fmt->h_align,
>>> + &q_data->h_adjusted,
>>> + q_data->h_adjusted, /* adjust upwards */
>>> + MXC_JPEG_MAX_HEIGHT,
>>> + q_data->fmt->v_align,
>>> + 0);
>>>
>>> for (i = 0; i < pix_mp->num_planes; i++) {
>>> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-06-16 7:30 ` mirela.rabulea
0 siblings, 0 replies; 10+ messages in thread
From: mirela.rabulea @ 2022-06-16 7:30 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
Hi Ming,
On 13.06.2022 08:25, Ming Qian wrote:
>> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
>> Sent: 2022年6月13日 6:56
>> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
>> hverkuil-cisco@xs4all.nl
>> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
>> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>>
>> Hi Ming,
>>
>> On 30.05.2022 10:49, Ming Qian wrote:
>>> The hardware can support any image size WxH, with arbitrary W (image
>>> width) and H (image height) dimensions.
>>>
>>> Align upwards buffer size for both encoder and decoder.
>>> and leave the picture resolution unchanged.
>>>
>>> For decoder, the risk of memory out of bounds can be avoided.
>>> For both encoder and decoder, the driver will lift the limitation of
>>> resolution alignment.
>>>
>>> For example, the decoder can support jpeg whose resolution is 227x149
>>
>> I doubt 227x149 is working. I have tried 127x127, with your patch applied,
>> both on encoder and decoder, the image does not look ok. The
>> 126x127 seems to work. Having an odd resolution seems strange, I see not
>> even gstreamer supports it (tried videotestsrc & filesink with BGR,
>> 127x128 produces a 128x128 size).
>>
>> We need to gain more clarity on this one.
>> And when we do, if we really can support any arbitrary resolution, from both
>> the jpeg core and wrapper point of view, we have stuff to clean up.
>> The assumption that I started with was, as stated in the comments:
>> * The alignment requirements for the resolution depend on the format,
>> * multiple of 16 resolutions should work for all formats.
>> * Special workarounds are made in the driver to support NV12 1080p.
>> With h_align/v_align defined in mxc_formats[].
>>
>> Regards,
>> Mirela
>
> Hi Mirela,
> I think you are confusing picture size and buffer size.
> In this patch, driver will enlarge the buffer size to align 16x16. But let the picture size unchanged.
> And if you display the decoded 227x149 picture directly on imx8q, you may meet some drm error, and the display looks abnormal,
> The error message like below:
> [ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
> [ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen requests to read empty FIFO
> [ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for content shdld done timeout
>
> But if you save the decoded picture data in to a file, then check the data, you will find it's correct.
> And if you treat the decoded buffer as a picture with resolution 240x160, and with some padding content, it's also correct.
>
> So in my first patch, I let the driver report the aligned resolution in g_fmt,
> and implement a g_selection to report the actual resolution through the crop info.
> but this solution will fail your labgrid jpeg test.
>
> So I choose the current solution that keep the actual picture size, and align upwards the buffer size.
>
> The display of 227x149 is abnormal, I think it's not the jpeg codec's limitation, but the imx8q drm's limitation.
I did not try to display with gstreamer. I just tried to generate some
test files. For example, this one:
gst-launch-1.0 videotestsrc pattern=smpte75 num-buffers=1 !
video/x-raw,width=227,height=149,format=BGR ! filesink
location=bgr_227x149.raw
generates a 228x149 file, not 227x149, with the 228 column black
(padding?). For visualizing, I used vooya, on host machine.
I then tried the pattern generator: https://github.com/NXPmicro/nxp-patgen
./patgen.exe -pix_fmt yuv444 -p colorbar -vsize 227x149
This generates a raw yuv444 227x149, without padding.
I used the unit test application to encode it.
The resulting jpeg is reported by JPEGSnoop to have Image Size =
227x149, and it looks bad, every line is shifted, as if it would have
228 width.
I did not check yet to see if any adjustments are needed in the unit test.
I'll send you my test files.
Regards,
Mirela
>
> And in my opinion, I prefer the first solution that implementing a g_selection to report the actual picture size.
> If you can accept that changing your labgrid jpeg testcase, I can prepare a v3 patch to switch to this solution.
>
> Ming
>
>>
>>> the encoder can support nv12 1080P, won't change it to 1920x1072.
>>>
>>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG
>>> Encoder/Decoder")
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v2
>>> - add Fixes tag
>>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88 ++++++++-----------
>>> 1 file changed, 37 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> index c0fd030d0f19..9a1c8df522ed 100644
>>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>>> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
>> vb2_buffer *out_buf,
>>> jpeg->slot_data[slot].cfg_stream_size =
>>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
>>> q_data->fmt->fourcc,
>>> - q_data->w_adjusted,
>>> - q_data->h_adjusted);
>>> + q_data->w,
>>> + q_data->h);
>>>
>>> /* chain the config descriptor with the encoding descriptor */
>>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; @@
>>> -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx
>> *ctx,
>>> &q_data_cap->h_adjusted,
>>> q_data_cap->h_adjusted, /* adjust up */
>>> MXC_JPEG_MAX_HEIGHT,
>>> - q_data_cap->fmt->v_align,
>>> + 0,
>>> 0);
>>>
>>> /* setup bytesperline/sizeimage for capture queue */ @@ -1161,18
>>> +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
>>> {
>>> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
>>> struct mxc_jpeg_q_data *q_data = NULL;
>>> + struct mxc_jpeg_q_data tmp_q;
>>> int i;
>>>
>>> q_data = mxc_jpeg_get_q_data(ctx, q->type);
>>> if (!q_data)
>>> return -EINVAL;
>>>
>>> + tmp_q.fmt = q_data->fmt;
>>> + tmp_q.w = q_data->w_adjusted;
>>> + tmp_q.h = q_data->h_adjusted;
>>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
>>> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
>>> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
>>> + }
>>> + mxc_jpeg_sizeimage(&tmp_q);
>>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
>>> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i], q_data->sizeimage[i]);
>>> +
>>> /* Handle CREATE_BUFS situation - *nplanes != 0 */
>>> if (*nplanes) {
>>> if (*nplanes != q_data->fmt->colplanes)
>>> return -EINVAL;
>>> for (i = 0; i < *nplanes; i++) {
>>> - if (sizes[i] < q_data->sizeimage[i])
>>> + if (sizes[i] < tmp_q.sizeimage[i])
>>> return -EINVAL;
>>> }
>>> return 0;
>>> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
>> vb2_queue *q,
>>> /* Handle REQBUFS situation */
>>> *nplanes = q_data->fmt->colplanes;
>>> for (i = 0; i < *nplanes; i++)
>>> - sizes[i] = q_data->sizeimage[i];
>>> + sizes[i] = tmp_q.sizeimage[i];
>>>
>>> return 0;
>>> }
>>> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
>> *ctx, struct vb2_buffer *vb)
>>> }
>>> q_data_out->w = header.frame.width;
>>> q_data_out->h = header.frame.height;
>>> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
>>> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
>>> - header.frame.width, header.frame.height);
>>> - return -EINVAL;
>>> - }
>>> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
>>> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
>>> dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
>> @@
>>> -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format *f,
>> const struct mxc_jpeg_fmt *fm
>>> pix_mp->num_planes = fmt->colplanes;
>>> pix_mp->pixelformat = fmt->fourcc;
>>>
>>> - /*
>>> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
>>> - * alignment, to loosen up the alignment to multiple of 8,
>>> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
>>> - */
>>> + pix_mp->width = w;
>>> + pix_mp->height = h;
>>> v4l_bound_align_image(&w,
>>> - MXC_JPEG_MIN_WIDTH,
>>> - w, /* adjust downwards*/
>>> + w, /* adjust upwards*/
>>> + MXC_JPEG_MAX_WIDTH,
>>> fmt->h_align,
>>> &h,
>>> - MXC_JPEG_MIN_HEIGHT,
>>> - h, /* adjust downwards*/
>>> - MXC_JPEG_H_ALIGN,
>>> + h, /* adjust upwards*/
>>> + MXC_JPEG_MAX_HEIGHT,
>>> + 0,
>>> 0);
>>> - pix_mp->width = w; /* negotiate the width */
>>> - pix_mp->height = h; /* negotiate the height */
>>>
>>> /* get user input into the tmp_q */
>>> tmp_q.w = w;
>>> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct mxc_jpeg_ctx
>>> *ctx,
>>>
>>> q_data->w_adjusted = q_data->w;
>>> q_data->h_adjusted = q_data->h;
>>> - if (jpeg->mode == MXC_JPEG_DECODE) {
>>> - /*
>>> - * align up the resolution for CAST IP,
>>> - * but leave the buffer resolution unchanged
>>> - */
>>> - v4l_bound_align_image(&q_data->w_adjusted,
>>> - q_data->w_adjusted, /* adjust upwards */
>>> - MXC_JPEG_MAX_WIDTH,
>>> - q_data->fmt->h_align,
>>> - &q_data->h_adjusted,
>>> - q_data->h_adjusted, /* adjust upwards */
>>> - MXC_JPEG_MAX_HEIGHT,
>>> - q_data->fmt->v_align,
>>> - 0);
>>> - } else {
>>> - /*
>>> - * align down the resolution for CAST IP,
>>> - * but leave the buffer resolution unchanged
>>> - */
>>> - v4l_bound_align_image(&q_data->w_adjusted,
>>> - MXC_JPEG_MIN_WIDTH,
>>> - q_data->w_adjusted, /* adjust downwards*/
>>> - q_data->fmt->h_align,
>>> - &q_data->h_adjusted,
>>> - MXC_JPEG_MIN_HEIGHT,
>>> - q_data->h_adjusted, /* adjust downwards*/
>>> - q_data->fmt->v_align,
>>> - 0);
>>> - }
>>> + /*
>>> + * align up the resolution for CAST IP,
>>> + * but leave the buffer resolution unchanged
>>> + */
>>> + v4l_bound_align_image(&q_data->w_adjusted,
>>> + q_data->w_adjusted, /* adjust upwards */
>>> + MXC_JPEG_MAX_WIDTH,
>>> + q_data->fmt->h_align,
>>> + &q_data->h_adjusted,
>>> + q_data->h_adjusted, /* adjust upwards */
>>> + MXC_JPEG_MAX_HEIGHT,
>>> + q_data->fmt->v_align,
>>> + 0);
>>>
>>> for (i = 0; i < pix_mp->num_planes; i++) {
>>> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2] media: imx-jpeg: Align upwards buffer size
2022-06-16 7:30 ` mirela.rabulea
@ 2022-06-16 9:03 ` Ming Qian
-1 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-06-16 9:03 UTC (permalink / raw)
To: Mirela Rabulea (OSS), mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> Sent: 2022年6月16日 15:31
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> hverkuil-cisco@xs4all.nl
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>
> Hi Ming,
>
> On 13.06.2022 08:25, Ming Qian wrote:
> >> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> >> Sent: 2022年6月13日 6:56
> >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> >> hverkuil-cisco@xs4all.nl
> >> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> >> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> >> <linux-imx@nxp.com>; linux-media@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
> >>
> >> Hi Ming,
> >>
> >> On 30.05.2022 10:49, Ming Qian wrote:
> >>> The hardware can support any image size WxH, with arbitrary W (image
> >>> width) and H (image height) dimensions.
> >>>
> >>> Align upwards buffer size for both encoder and decoder.
> >>> and leave the picture resolution unchanged.
> >>>
> >>> For decoder, the risk of memory out of bounds can be avoided.
> >>> For both encoder and decoder, the driver will lift the limitation of
> >>> resolution alignment.
> >>>
> >>> For example, the decoder can support jpeg whose resolution is
> >>> 227x149
> >>
> >> I doubt 227x149 is working. I have tried 127x127, with your patch
> >> applied, both on encoder and decoder, the image does not look ok. The
> >> 126x127 seems to work. Having an odd resolution seems strange, I see
> >> not even gstreamer supports it (tried videotestsrc & filesink with
> >> BGR,
> >> 127x128 produces a 128x128 size).
> >>
> >> We need to gain more clarity on this one.
> >> And when we do, if we really can support any arbitrary resolution,
> >> from both the jpeg core and wrapper point of view, we have stuff to clean
> up.
> >> The assumption that I started with was, as stated in the comments:
> >> * The alignment requirements for the resolution depend on the format,
> >> * multiple of 16 resolutions should work for all formats.
> >> * Special workarounds are made in the driver to support NV12 1080p.
> >> With h_align/v_align defined in mxc_formats[].
> >>
> >> Regards,
> >> Mirela
> >
> > Hi Mirela,
> > I think you are confusing picture size and buffer size.
> > In this patch, driver will enlarge the buffer size to align 16x16. But let
> the picture size unchanged.
> > And if you display the decoded 227x149 picture directly on imx8q,
> > you may meet some drm error, and the display looks abnormal, The error
> message like below:
> > [ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for
> content shdld done timeout
> > [ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen
> requests to read empty FIFO
> > [ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for
> content shdld done timeout
> >
> > But if you save the decoded picture data in to a file, then check the data, you
> will find it's correct.
> > And if you treat the decoded buffer as a picture with resolution 240x160, and
> with some padding content, it's also correct.
> >
> > So in my first patch, I let the driver report the aligned resolution
> > in g_fmt, and implement a g_selection to report the actual resolution
> through the crop info.
> > but this solution will fail your labgrid jpeg test.
> >
> > So I choose the current solution that keep the actual picture size, and align
> upwards the buffer size.
> >
> > The display of 227x149 is abnormal, I think it's not the jpeg codec's
> limitation, but the imx8q drm's limitation.
>
> I did not try to display with gstreamer. I just tried to generate some test files.
> For example, this one:
> gst-launch-1.0 videotestsrc pattern=smpte75 num-buffers=1 !
> video/x-raw,width=227,height=149,format=BGR ! filesink
> location=bgr_227x149.raw generates a 228x149 file, not 227x149, with the
> 228 column black (padding?). For visualizing, I used vooya, on host machine.
>
> I then tried the pattern generator: https://github.com/NXPmicro/nxp-patgen
> ./patgen.exe -pix_fmt yuv444 -p colorbar -vsize 227x149
>
> This generates a raw yuv444 227x149, without padding.
> I used the unit test application to encode it.
> The resulting jpeg is reported by JPEGSnoop to have Image Size = 227x149,
> and it looks bad, every line is shifted, as if it would have
> 228 width.
> I did not check yet to see if any adjustments are needed in the unit test.
>
> I'll send you my test files.
>
> Regards,
> Mirela
>
Hi Mirela,
I tested your test file patgen-colorbar-227x149-yuv444.yuv, and encode it to jpeg.
The encoded jpeg has image size 227x149, and it looks good. I have sent it to you, you can double check.
Note when encoding the 227x149 yuv444 image, the buffer size is aligned upwards to 696 x 152.
So when you write the yuv date into the buffer, you should write line by line. The bytesperline of the buffer is not 681(227x3), but 696.
Ming
> >
> > And in my opinion, I prefer the first solution that implementing a g_selection
> to report the actual picture size.
> > If you can accept that changing your labgrid jpeg testcase, I can prepare a v3
> patch to switch to this solution.
> >
> > Ming
> >
> >>
> >>> the encoder can support nv12 1080P, won't change it to 1920x1072.
> >>>
> >>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8
> >>> JPEG
> >>> Encoder/Decoder")
> >>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> >>> ---
> >>> v2
> >>> - add Fixes tag
> >>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88
> ++++++++-----------
> >>> 1 file changed, 37 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> index c0fd030d0f19..9a1c8df522ed 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
> >> vb2_buffer *out_buf,
> >>> jpeg->slot_data[slot].cfg_stream_size =
> >>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> >>> q_data->fmt->fourcc,
> >>> - q_data->w_adjusted,
> >>> - q_data->h_adjusted);
> >>> + q_data->w,
> >>> + q_data->h);
> >>>
> >>> /* chain the config descriptor with the encoding descriptor */
> >>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
> @@
> >>> -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct
> >>> mxc_jpeg_ctx
> >> *ctx,
> >>> &q_data_cap->h_adjusted,
> >>> q_data_cap->h_adjusted, /* adjust up */
> >>> MXC_JPEG_MAX_HEIGHT,
> >>> - q_data_cap->fmt->v_align,
> >>> + 0,
> >>> 0);
> >>>
> >>> /* setup bytesperline/sizeimage for capture queue */ @@
> >>> -1161,18
> >>> +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> >>> {
> >>> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> >>> struct mxc_jpeg_q_data *q_data = NULL;
> >>> + struct mxc_jpeg_q_data tmp_q;
> >>> int i;
> >>>
> >>> q_data = mxc_jpeg_get_q_data(ctx, q->type);
> >>> if (!q_data)
> >>> return -EINVAL;
> >>>
> >>> + tmp_q.fmt = q_data->fmt;
> >>> + tmp_q.w = q_data->w_adjusted;
> >>> + tmp_q.h = q_data->h_adjusted;
> >>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> >>> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> >>> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> >>> + }
> >>> + mxc_jpeg_sizeimage(&tmp_q);
> >>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> >>> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i],
> >>> +q_data->sizeimage[i]);
> >>> +
> >>> /* Handle CREATE_BUFS situation - *nplanes != 0 */
> >>> if (*nplanes) {
> >>> if (*nplanes != q_data->fmt->colplanes)
> >>> return -EINVAL;
> >>> for (i = 0; i < *nplanes; i++) {
> >>> - if (sizes[i] < q_data->sizeimage[i])
> >>> + if (sizes[i] < tmp_q.sizeimage[i])
> >>> return -EINVAL;
> >>> }
> >>> return 0;
> >>> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
> >> vb2_queue *q,
> >>> /* Handle REQBUFS situation */
> >>> *nplanes = q_data->fmt->colplanes;
> >>> for (i = 0; i < *nplanes; i++)
> >>> - sizes[i] = q_data->sizeimage[i];
> >>> + sizes[i] = tmp_q.sizeimage[i];
> >>>
> >>> return 0;
> >>> }
> >>> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
> >> *ctx, struct vb2_buffer *vb)
> >>> }
> >>> q_data_out->w = header.frame.width;
> >>> q_data_out->h = header.frame.height;
> >>> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> >>> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> >>> - header.frame.width, header.frame.height);
> >>> - return -EINVAL;
> >>> - }
> >>> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> >>> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> >>> dev_err(dev, "JPEG width or height should be <=
> 8192: %dx%d\n",
> >> @@
> >>> -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format
> >>> *f,
> >> const struct mxc_jpeg_fmt *fm
> >>> pix_mp->num_planes = fmt->colplanes;
> >>> pix_mp->pixelformat = fmt->fourcc;
> >>>
> >>> - /*
> >>> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> >>> - * alignment, to loosen up the alignment to multiple of 8,
> >>> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> >>> - */
> >>> + pix_mp->width = w;
> >>> + pix_mp->height = h;
> >>> v4l_bound_align_image(&w,
> >>> - MXC_JPEG_MIN_WIDTH,
> >>> - w, /* adjust downwards*/
> >>> + w, /* adjust upwards*/
> >>> + MXC_JPEG_MAX_WIDTH,
> >>> fmt->h_align,
> >>> &h,
> >>> - MXC_JPEG_MIN_HEIGHT,
> >>> - h, /* adjust downwards*/
> >>> - MXC_JPEG_H_ALIGN,
> >>> + h, /* adjust upwards*/
> >>> + MXC_JPEG_MAX_HEIGHT,
> >>> + 0,
> >>> 0);
> >>> - pix_mp->width = w; /* negotiate the width */
> >>> - pix_mp->height = h; /* negotiate the height */
> >>>
> >>> /* get user input into the tmp_q */
> >>> tmp_q.w = w;
> >>> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct
> >>> mxc_jpeg_ctx *ctx,
> >>>
> >>> q_data->w_adjusted = q_data->w;
> >>> q_data->h_adjusted = q_data->h;
> >>> - if (jpeg->mode == MXC_JPEG_DECODE) {
> >>> - /*
> >>> - * align up the resolution for CAST IP,
> >>> - * but leave the buffer resolution unchanged
> >>> - */
> >>> - v4l_bound_align_image(&q_data->w_adjusted,
> >>> - q_data->w_adjusted, /* adjust upwards */
> >>> - MXC_JPEG_MAX_WIDTH,
> >>> - q_data->fmt->h_align,
> >>> - &q_data->h_adjusted,
> >>> - q_data->h_adjusted, /* adjust upwards */
> >>> - MXC_JPEG_MAX_HEIGHT,
> >>> - q_data->fmt->v_align,
> >>> - 0);
> >>> - } else {
> >>> - /*
> >>> - * align down the resolution for CAST IP,
> >>> - * but leave the buffer resolution unchanged
> >>> - */
> >>> - v4l_bound_align_image(&q_data->w_adjusted,
> >>> - MXC_JPEG_MIN_WIDTH,
> >>> - q_data->w_adjusted, /* adjust downwards*/
> >>> - q_data->fmt->h_align,
> >>> - &q_data->h_adjusted,
> >>> - MXC_JPEG_MIN_HEIGHT,
> >>> - q_data->h_adjusted, /* adjust downwards*/
> >>> - q_data->fmt->v_align,
> >>> - 0);
> >>> - }
> >>> + /*
> >>> + * align up the resolution for CAST IP,
> >>> + * but leave the buffer resolution unchanged
> >>> + */
> >>> + v4l_bound_align_image(&q_data->w_adjusted,
> >>> + q_data->w_adjusted, /* adjust upwards */
> >>> + MXC_JPEG_MAX_WIDTH,
> >>> + q_data->fmt->h_align,
> >>> + &q_data->h_adjusted,
> >>> + q_data->h_adjusted, /* adjust upwards */
> >>> + MXC_JPEG_MAX_HEIGHT,
> >>> + q_data->fmt->v_align,
> >>> + 0);
> >>>
> >>> for (i = 0; i < pix_mp->num_planes; i++) {
> >>> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2] media: imx-jpeg: Align upwards buffer size
@ 2022-06-16 9:03 ` Ming Qian
0 siblings, 0 replies; 10+ messages in thread
From: Ming Qian @ 2022-06-16 9:03 UTC (permalink / raw)
To: Mirela Rabulea (OSS), mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> Sent: 2022年6月16日 15:31
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> hverkuil-cisco@xs4all.nl
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
>
> Hi Ming,
>
> On 13.06.2022 08:25, Ming Qian wrote:
> >> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>
> >> Sent: 2022年6月13日 6:56
> >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> >> hverkuil-cisco@xs4all.nl
> >> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> >> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> >> <linux-imx@nxp.com>; linux-media@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size
> >>
> >> Hi Ming,
> >>
> >> On 30.05.2022 10:49, Ming Qian wrote:
> >>> The hardware can support any image size WxH, with arbitrary W (image
> >>> width) and H (image height) dimensions.
> >>>
> >>> Align upwards buffer size for both encoder and decoder.
> >>> and leave the picture resolution unchanged.
> >>>
> >>> For decoder, the risk of memory out of bounds can be avoided.
> >>> For both encoder and decoder, the driver will lift the limitation of
> >>> resolution alignment.
> >>>
> >>> For example, the decoder can support jpeg whose resolution is
> >>> 227x149
> >>
> >> I doubt 227x149 is working. I have tried 127x127, with your patch
> >> applied, both on encoder and decoder, the image does not look ok. The
> >> 126x127 seems to work. Having an odd resolution seems strange, I see
> >> not even gstreamer supports it (tried videotestsrc & filesink with
> >> BGR,
> >> 127x128 produces a 128x128 size).
> >>
> >> We need to gain more clarity on this one.
> >> And when we do, if we really can support any arbitrary resolution,
> >> from both the jpeg core and wrapper point of view, we have stuff to clean
> up.
> >> The assumption that I started with was, as stated in the comments:
> >> * The alignment requirements for the resolution depend on the format,
> >> * multiple of 16 resolutions should work for all formats.
> >> * Special workarounds are made in the driver to support NV12 1080p.
> >> With h_align/v_align defined in mxc_formats[].
> >>
> >> Regards,
> >> Mirela
> >
> > Hi Mirela,
> > I think you are confusing picture size and buffer size.
> > In this patch, driver will enlarge the buffer size to align 16x16. But let
> the picture size unchanged.
> > And if you display the decoded 227x149 picture directly on imx8q,
> > you may meet some drm error, and the display looks abnormal, The error
> message like below:
> > [ 36.381015] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for
> content shdld done timeout
> > [ 36.389630] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: FrameGen
> requests to read empty FIFO
> > [ 49.469022] [drm] [CRTC:38:crtc-0] dpu_crtc_atomic_flush: wait for
> content shdld done timeout
> >
> > But if you save the decoded picture data in to a file, then check the data, you
> will find it's correct.
> > And if you treat the decoded buffer as a picture with resolution 240x160, and
> with some padding content, it's also correct.
> >
> > So in my first patch, I let the driver report the aligned resolution
> > in g_fmt, and implement a g_selection to report the actual resolution
> through the crop info.
> > but this solution will fail your labgrid jpeg test.
> >
> > So I choose the current solution that keep the actual picture size, and align
> upwards the buffer size.
> >
> > The display of 227x149 is abnormal, I think it's not the jpeg codec's
> limitation, but the imx8q drm's limitation.
>
> I did not try to display with gstreamer. I just tried to generate some test files.
> For example, this one:
> gst-launch-1.0 videotestsrc pattern=smpte75 num-buffers=1 !
> video/x-raw,width=227,height=149,format=BGR ! filesink
> location=bgr_227x149.raw generates a 228x149 file, not 227x149, with the
> 228 column black (padding?). For visualizing, I used vooya, on host machine.
>
> I then tried the pattern generator: https://github.com/NXPmicro/nxp-patgen
> ./patgen.exe -pix_fmt yuv444 -p colorbar -vsize 227x149
>
> This generates a raw yuv444 227x149, without padding.
> I used the unit test application to encode it.
> The resulting jpeg is reported by JPEGSnoop to have Image Size = 227x149,
> and it looks bad, every line is shifted, as if it would have
> 228 width.
> I did not check yet to see if any adjustments are needed in the unit test.
>
> I'll send you my test files.
>
> Regards,
> Mirela
>
Hi Mirela,
I tested your test file patgen-colorbar-227x149-yuv444.yuv, and encode it to jpeg.
The encoded jpeg has image size 227x149, and it looks good. I have sent it to you, you can double check.
Note when encoding the 227x149 yuv444 image, the buffer size is aligned upwards to 696 x 152.
So when you write the yuv date into the buffer, you should write line by line. The bytesperline of the buffer is not 681(227x3), but 696.
Ming
> >
> > And in my opinion, I prefer the first solution that implementing a g_selection
> to report the actual picture size.
> > If you can accept that changing your labgrid jpeg testcase, I can prepare a v3
> patch to switch to this solution.
> >
> > Ming
> >
> >>
> >>> the encoder can support nv12 1080P, won't change it to 1920x1072.
> >>>
> >>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8
> >>> JPEG
> >>> Encoder/Decoder")
> >>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> >>> ---
> >>> v2
> >>> - add Fixes tag
> >>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 88
> ++++++++-----------
> >>> 1 file changed, 37 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> index c0fd030d0f19..9a1c8df522ed 100644
> >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> >>> @@ -894,8 +894,8 @@ static void mxc_jpeg_config_enc_desc(struct
> >> vb2_buffer *out_buf,
> >>> jpeg->slot_data[slot].cfg_stream_size =
> >>> mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr,
> >>> q_data->fmt->fourcc,
> >>> - q_data->w_adjusted,
> >>> - q_data->h_adjusted);
> >>> + q_data->w,
> >>> + q_data->h);
> >>>
> >>> /* chain the config descriptor with the encoding descriptor */
> >>> cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN;
> @@
> >>> -977,7 +977,7 @@ static bool mxc_jpeg_source_change(struct
> >>> mxc_jpeg_ctx
> >> *ctx,
> >>> &q_data_cap->h_adjusted,
> >>> q_data_cap->h_adjusted, /* adjust up */
> >>> MXC_JPEG_MAX_HEIGHT,
> >>> - q_data_cap->fmt->v_align,
> >>> + 0,
> >>> 0);
> >>>
> >>> /* setup bytesperline/sizeimage for capture queue */ @@
> >>> -1161,18
> >>> +1161,30 @@ static int mxc_jpeg_queue_setup(struct vb2_queue *q,
> >>> {
> >>> struct mxc_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> >>> struct mxc_jpeg_q_data *q_data = NULL;
> >>> + struct mxc_jpeg_q_data tmp_q;
> >>> int i;
> >>>
> >>> q_data = mxc_jpeg_get_q_data(ctx, q->type);
> >>> if (!q_data)
> >>> return -EINVAL;
> >>>
> >>> + tmp_q.fmt = q_data->fmt;
> >>> + tmp_q.w = q_data->w_adjusted;
> >>> + tmp_q.h = q_data->h_adjusted;
> >>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++) {
> >>> + tmp_q.bytesperline[i] = q_data->bytesperline[i];
> >>> + tmp_q.sizeimage[i] = q_data->sizeimage[i];
> >>> + }
> >>> + mxc_jpeg_sizeimage(&tmp_q);
> >>> + for (i = 0; i < MXC_JPEG_MAX_PLANES; i++)
> >>> + tmp_q.sizeimage[i] = max(tmp_q.sizeimage[i],
> >>> +q_data->sizeimage[i]);
> >>> +
> >>> /* Handle CREATE_BUFS situation - *nplanes != 0 */
> >>> if (*nplanes) {
> >>> if (*nplanes != q_data->fmt->colplanes)
> >>> return -EINVAL;
> >>> for (i = 0; i < *nplanes; i++) {
> >>> - if (sizes[i] < q_data->sizeimage[i])
> >>> + if (sizes[i] < tmp_q.sizeimage[i])
> >>> return -EINVAL;
> >>> }
> >>> return 0;
> >>> @@ -1181,7 +1193,7 @@ static int mxc_jpeg_queue_setup(struct
> >> vb2_queue *q,
> >>> /* Handle REQBUFS situation */
> >>> *nplanes = q_data->fmt->colplanes;
> >>> for (i = 0; i < *nplanes; i++)
> >>> - sizes[i] = q_data->sizeimage[i];
> >>> + sizes[i] = tmp_q.sizeimage[i];
> >>>
> >>> return 0;
> >>> }
> >>> @@ -1381,11 +1393,6 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx
> >> *ctx, struct vb2_buffer *vb)
> >>> }
> >>> q_data_out->w = header.frame.width;
> >>> q_data_out->h = header.frame.height;
> >>> - if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
> >>> - dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> >>> - header.frame.width, header.frame.height);
> >>> - return -EINVAL;
> >>> - }
> >>> if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> >>> header.frame.height > MXC_JPEG_MAX_HEIGHT) {
> >>> dev_err(dev, "JPEG width or height should be <=
> 8192: %dx%d\n",
> >> @@
> >>> -1748,22 +1755,17 @@ static int mxc_jpeg_try_fmt(struct v4l2_format
> >>> *f,
> >> const struct mxc_jpeg_fmt *fm
> >>> pix_mp->num_planes = fmt->colplanes;
> >>> pix_mp->pixelformat = fmt->fourcc;
> >>>
> >>> - /*
> >>> - * use MXC_JPEG_H_ALIGN instead of fmt->v_align, for vertical
> >>> - * alignment, to loosen up the alignment to multiple of 8,
> >>> - * otherwise NV12-1080p fails as 1080 is not a multiple of 16
> >>> - */
> >>> + pix_mp->width = w;
> >>> + pix_mp->height = h;
> >>> v4l_bound_align_image(&w,
> >>> - MXC_JPEG_MIN_WIDTH,
> >>> - w, /* adjust downwards*/
> >>> + w, /* adjust upwards*/
> >>> + MXC_JPEG_MAX_WIDTH,
> >>> fmt->h_align,
> >>> &h,
> >>> - MXC_JPEG_MIN_HEIGHT,
> >>> - h, /* adjust downwards*/
> >>> - MXC_JPEG_H_ALIGN,
> >>> + h, /* adjust upwards*/
> >>> + MXC_JPEG_MAX_HEIGHT,
> >>> + 0,
> >>> 0);
> >>> - pix_mp->width = w; /* negotiate the width */
> >>> - pix_mp->height = h; /* negotiate the height */
> >>>
> >>> /* get user input into the tmp_q */
> >>> tmp_q.w = w;
> >>> @@ -1889,35 +1891,19 @@ static int mxc_jpeg_s_fmt(struct
> >>> mxc_jpeg_ctx *ctx,
> >>>
> >>> q_data->w_adjusted = q_data->w;
> >>> q_data->h_adjusted = q_data->h;
> >>> - if (jpeg->mode == MXC_JPEG_DECODE) {
> >>> - /*
> >>> - * align up the resolution for CAST IP,
> >>> - * but leave the buffer resolution unchanged
> >>> - */
> >>> - v4l_bound_align_image(&q_data->w_adjusted,
> >>> - q_data->w_adjusted, /* adjust upwards */
> >>> - MXC_JPEG_MAX_WIDTH,
> >>> - q_data->fmt->h_align,
> >>> - &q_data->h_adjusted,
> >>> - q_data->h_adjusted, /* adjust upwards */
> >>> - MXC_JPEG_MAX_HEIGHT,
> >>> - q_data->fmt->v_align,
> >>> - 0);
> >>> - } else {
> >>> - /*
> >>> - * align down the resolution for CAST IP,
> >>> - * but leave the buffer resolution unchanged
> >>> - */
> >>> - v4l_bound_align_image(&q_data->w_adjusted,
> >>> - MXC_JPEG_MIN_WIDTH,
> >>> - q_data->w_adjusted, /* adjust downwards*/
> >>> - q_data->fmt->h_align,
> >>> - &q_data->h_adjusted,
> >>> - MXC_JPEG_MIN_HEIGHT,
> >>> - q_data->h_adjusted, /* adjust downwards*/
> >>> - q_data->fmt->v_align,
> >>> - 0);
> >>> - }
> >>> + /*
> >>> + * align up the resolution for CAST IP,
> >>> + * but leave the buffer resolution unchanged
> >>> + */
> >>> + v4l_bound_align_image(&q_data->w_adjusted,
> >>> + q_data->w_adjusted, /* adjust upwards */
> >>> + MXC_JPEG_MAX_WIDTH,
> >>> + q_data->fmt->h_align,
> >>> + &q_data->h_adjusted,
> >>> + q_data->h_adjusted, /* adjust upwards */
> >>> + MXC_JPEG_MAX_HEIGHT,
> >>> + q_data->fmt->v_align,
> >>> + 0);
> >>>
> >>> for (i = 0; i < pix_mp->num_planes; i++) {
> >>> q_data->bytesperline[i] = pix_mp->plane_fmt[i].bytesperline;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-16 9:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-30 7:49 [PATCH v2] media: imx-jpeg: Align upwards buffer size Ming Qian
2022-05-30 7:49 ` Ming Qian
2022-06-12 22:55 ` mirela.rabulea
2022-06-12 22:55 ` mirela.rabulea
2022-06-13 5:25 ` Ming Qian
2022-06-13 5:25 ` Ming Qian
2022-06-16 7:30 ` mirela.rabulea
2022-06-16 7:30 ` mirela.rabulea
2022-06-16 9:03 ` Ming Qian
2022-06-16 9:03 ` Ming Qian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.