[PATCH v11 03/12] media: mediatek: jpeg: fix jpeg buffer layout

[Nicolas Dufresne]

Le jeudi 25 décembre 2025 à 06:05 +0000, Kyrie Wu (吴晗) a écrit :
> On Tue, 2025-12-16 at 16:22 -0500, Nicolas Dufresne wrote:
> > Hi,
> > 
> > Le mardi 02 décembre 2025 à 17:47 +0800, Kyrie Wu a écrit :
> > > For memory alloc operation of jpeg dst buffer: the mallocing
> > > memory function interface use vb2_buffer as the base addr.
> > > If structure mtk_jpeg_src_buf wants to be allocated to memory,
> > > it needs to be placed vb2_v4l2_buffer at the starting position,
> > > because structure vb2_buffer is at the starting position of
> > > vb2_v4l2_buffer, and the allocated size is set to the size of
> > > structure mtk_jpeg_src_buf, so as to ensure that structures
> > > mtk_jpeg_src_buf, vb2_v4l2_buffer and vb2_buffer can all be
> > > allocated memory.
> > 
> > Please correct the wording, "mallocing" is not a word, addr ->
> > address. I tend
> > to do the same, but refrain from giving the code a personality, the
> > vb2_buffer
> > have no will. This is overall complicated way to simply say:
> > 
> > 	Fix the size of the allocated mtk_jpeg_src_buf structure.
> > 
> > > 
> > > Fixes: 5fb1c2361e56 ("mtk-jpegenc: add jpeg encode worker
> > > interface")
> > > 
> > 
> > Drop the blank line.
> > 
> > > Signed-off-by: Kyrie Wu <kyrie.wu at mediatek.com>
> > > ---
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +-
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > index 0cf3dc5407e4..bd0afc93d491 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > @@ -1099,7 +1099,7 @@ static int mtk_jpeg_queue_init(void *priv,
> > > struct vb2_queue *src_vq,
> > >  	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > >  	dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
> > >  	dst_vq->drv_priv = ctx;
> > > -	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > > +	dst_vq->buf_struct_size = sizeof(struct mtk_jpeg_src_buf);
> > >  	dst_vq->ops = jpeg->variant->qops;
> > >  	dst_vq->mem_ops = &vb2_dma_contig_memops;
> > >  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > index 6be5cf30dea1..148fd41759b7 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > @@ -85,10 +85,10 @@ struct mtk_jpeg_variant {
> > >  };
> > >  
> > >  struct mtk_jpeg_src_buf {
> > > -	u32 frame_num;
> > >  	struct vb2_v4l2_buffer b;
> > >  	struct list_head list;
> > >  	u32 bs_size;
> > > +	u32 frame_num;
> > 
> > I like the change, but this shouldn't be an issue if you use
> > container_of, which
> > is safer then a plain cast. Please review the code and make sure to
> > use it. It
> > may be confusing to include a cosmetic change in a fixes.
> > 
> > Nicolas
> 
> Dear Nicolas,
> 
> The driver requests memory space for mtk_jpeg_src_buf in the
> function of mtk_jpeg_queue_init,
> and it used struct vb2_v4l2_buffer b as the starting base address,
> the parameter,frame_num, will not get a memory if we keep the old
> structure. And a unknown memory would be get if we used container_of
> to get the starting address of mtk_jpeg_src_buf.

You are right, and the documentation says so:

 *              The first field of the
 *		driver-specific buffer structure must be the subsystem-specific
 *		struct (vb2_v4l2_buffer in the case of V4L2).

This is inconsistent with how other objects are overloaded, notably the request
object, which does not have this limitation. We should probably fix that
someday, meanwhile.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

> 
> Thanks.
> 
> Regards,
> Kyrie.
> > 
> > >  	struct mtk_jpeg_dec_param dec_param;
> > >  
> > >  	struct mtk_jpeg_ctx *curr_ctx;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20260105/8f0335cb/attachment-0001.sig>