Re: [PATCH v3] media: imx-jpeg: Account for data_offset when getting image address

[Frank Li <Frank.li@nxp.com>]

On Wed, May 21, 2025 at 09:54:07AM +0800, ming.qian@oss.nxp.com wrote:
> From: Ming Qian <ming.qian@oss.nxp.com>
>
> Applications may set data_offset when it refers to an output queue. So
> driver need to account for it when getting the start address of input
> image in the plane.
>
> Meanwhile the mxc-jpeg codec requires the address (plane address +
> data_offset) to be 16-aligned.
>
> Fixes: 2db16c6ed72c ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
> v3
> - Add Fixes tag, it's a bug fix, as missed consider data_offset
>
> v2
> - Verify the address alignment
> ---
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 47 ++++++++++++++-----
>  .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  1 +
>  2 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 5c17bc58181e..8681dd193033 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -598,6 +598,27 @@ static void _bswap16(u16 *a)
>  	*a = ((*a & 0x00FF) << 8) | ((*a & 0xFF00) >> 8);
>  }
>
> +static dma_addr_t mxc_jpeg_get_plane_dma_addr(struct vb2_buffer *buf, unsigned int plane_no)
> +{
> +	if (plane_no >= buf->num_planes)
> +		return 0;
> +	return vb2_dma_contig_plane_dma_addr(buf, plane_no) + buf->planes[plane_no].data_offset;
> +}
> +
> +static void *mxc_jpeg_get_plane_vaddr(struct vb2_buffer *buf, unsigned int plane_no)
> +{
> +	if (plane_no >= buf->num_planes)
> +		return NULL;
> +	return vb2_plane_vaddr(buf, plane_no) + buf->planes[plane_no].data_offset;
> +}
> +
> +static unsigned long mxc_jpeg_get_plane_payload(struct vb2_buffer *buf, unsigned int plane_no)
> +{
> +	if (plane_no >= buf->num_planes)
> +		return 0;
> +	return vb2_get_plane_payload(buf, plane_no) - buf->planes[plane_no].data_offset;
> +}
> +
>  static void print_mxc_buf(struct mxc_jpeg_dev *jpeg, struct vb2_buffer *buf,
>  			  unsigned long len)
>  {
> @@ -610,11 +631,11 @@ static void print_mxc_buf(struct mxc_jpeg_dev *jpeg, struct vb2_buffer *buf,
>  		return;
>
>  	for (plane_no = 0; plane_no < buf->num_planes; plane_no++) {
> -		payload = vb2_get_plane_payload(buf, plane_no);
> +		payload = mxc_jpeg_get_plane_payload(buf, plane_no);
>  		if (len == 0)
>  			len = payload;
> -		dma_addr = vb2_dma_contig_plane_dma_addr(buf, plane_no);
> -		vaddr = vb2_plane_vaddr(buf, plane_no);
> +		dma_addr = mxc_jpeg_get_plane_dma_addr(buf, plane_no);
> +		vaddr = mxc_jpeg_get_plane_vaddr(buf, plane_no);
>  		v4l2_dbg(3, debug, &jpeg->v4l2_dev,
>  			 "plane %d (vaddr=%p dma_addr=%x payload=%ld):",
>  			  plane_no, vaddr, dma_addr, payload);
> @@ -712,16 +733,15 @@ static void mxc_jpeg_addrs(struct mxc_jpeg_desc *desc,
>  	struct mxc_jpeg_q_data *q_data;
>
>  	q_data = mxc_jpeg_get_q_data(ctx, raw_buf->type);
> -	desc->buf_base0 = vb2_dma_contig_plane_dma_addr(raw_buf, 0);
> +	desc->buf_base0 = mxc_jpeg_get_plane_dma_addr(raw_buf, 0);
>  	desc->buf_base1 = 0;
>  	if (img_fmt == STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV420)) {
>  		if (raw_buf->num_planes == 2)
> -			desc->buf_base1 = vb2_dma_contig_plane_dma_addr(raw_buf, 1);
> +			desc->buf_base1 = mxc_jpeg_get_plane_dma_addr(raw_buf, 1);
>  		else
>  			desc->buf_base1 = desc->buf_base0 + q_data->sizeimage[0];
>  	}
> -	desc->stm_bufbase = vb2_dma_contig_plane_dma_addr(jpeg_buf, 0) +
> -		offset;
> +	desc->stm_bufbase = mxc_jpeg_get_plane_dma_addr(jpeg_buf, 0) + offset;
>  }
>
>  static bool mxc_jpeg_is_extended_sequential(const struct mxc_jpeg_fmt *fmt)
> @@ -1029,8 +1049,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>  			vb2_set_plane_payload(&dst_buf->vb2_buf, 1, payload);
>  		}
>  		dev_dbg(dev, "Decoding finished, payload size: %ld + %ld\n",
> -			vb2_get_plane_payload(&dst_buf->vb2_buf, 0),
> -			vb2_get_plane_payload(&dst_buf->vb2_buf, 1));
> +			mxc_jpeg_get_plane_payload(&dst_buf->vb2_buf, 0),
> +			mxc_jpeg_get_plane_payload(&dst_buf->vb2_buf, 1));
>  	}
>
>  	/* short preview of the results */
> @@ -1889,8 +1909,8 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx, struct vb2_buffer *vb)
>  	struct mxc_jpeg_sof *psof = NULL;
>  	struct mxc_jpeg_sos *psos = NULL;
>  	struct mxc_jpeg_src_buf *jpeg_src_buf = vb2_to_mxc_buf(vb);
> -	u8 *src_addr = (u8 *)vb2_plane_vaddr(vb, 0);
> -	u32 size = vb2_get_plane_payload(vb, 0);
> +	u8 *src_addr = (u8 *)mxc_jpeg_get_plane_vaddr(vb, 0);
> +	u32 size = mxc_jpeg_get_plane_payload(vb, 0);
>  	int ret;
>
>  	memset(&header, 0, sizeof(header));
> @@ -2027,6 +2047,11 @@ static int mxc_jpeg_buf_prepare(struct vb2_buffer *vb)
>  				i, vb2_plane_size(vb, i), sizeimage);
>  			return -EINVAL;
>  		}
> +		if (!IS_ALIGNED(mxc_jpeg_get_plane_dma_addr(vb, i), MXC_JPEG_ADDR_ALIGNMENT)) {
> +			dev_err(dev, "planes[%d] address is not %d aligned\n",
> +				i, MXC_JPEG_ADDR_ALIGNMENT);
> +			return -EINVAL;
> +		}
>  	}
>  	if (V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type)) {
>  		vb2_set_plane_payload(vb, 0, 0);
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index fdde45f7e163..44e46face6d1 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -30,6 +30,7 @@
>  #define MXC_JPEG_MAX_PLANES		2
>  #define MXC_JPEG_PATTERN_WIDTH		128
>  #define MXC_JPEG_PATTERN_HEIGHT		64
> +#define MXC_JPEG_ADDR_ALIGNMENT		16
>
>  enum mxc_jpeg_enc_state {
>  	MXC_JPEG_ENCODING	= 0, /* jpeg encode phase */
>
> base-commit: b64b134942c8cf4801ea288b3fd38b509aedec21
> prerequisite-patch-id: 0000000000000000000000000000000000000000
> --
> 2.43.0-rc1
>