> From: Mirela Rabulea (OSS) <mirela.rabulea@xxxxxxxxxxx> > Sent: 2022年6月16日 15:31 > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; > hverkuil-cisco@xxxxxxxxx > Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxx> > >> Sent: 2022年6月13日 6:56 > >> To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; > >> hverkuil-cisco@xxxxxxxxx > >> Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > >> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx > >> <linux-imx@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > >> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >> 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@xxxxxxx> > >>> --- > >>> 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;
