[PATCH v2] media: imx-jpeg: Align upwards buffer size
Ming Qian
ming.qian at nxp.com
Thu Jun 16 02:03:53 PDT 2022
> From: Mirela Rabulea (OSS) <mirela.rabulea at oss.nxp.com>
> Sent: 2022年6月16日 15:31
> To: Ming Qian <ming.qian at nxp.com>; mchehab at kernel.org;
> hverkuil-cisco at xs4all.nl
> Cc: shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> festevam at gmail.com; dl-linux-imx <linux-imx at nxp.com>;
> linux-media at vger.kernel.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; linux-arm-kernel at 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 at oss.nxp.com>
> >> Sent: 2022年6月13日 6:56
> >> To: Ming Qian <ming.qian at nxp.com>; mchehab at kernel.org;
> >> hverkuil-cisco at xs4all.nl
> >> Cc: shawnguo at kernel.org; s.hauer at pengutronix.de;
> >> kernel at pengutronix.de; festevam at gmail.com; dl-linux-imx
> >> <linux-imx at nxp.com>; linux-media at vger.kernel.org;
> >> linux-kernel at vger.kernel.org; devicetree at vger.kernel.org;
> >> linux-arm-kernel at 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 at 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;
More information about the linux-arm-kernel
mailing list