Re: [PATCH v2] media: imx-jpeg: Align upwards buffer size

["mirela.rabulea@xxxxxxxxxxx" <mirela.rabulea@xxxxxxxxxxx>]

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

> 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;