Re: [PATCH v5 07/12] media: mediatek: jpeg: refactor jpeg dst buffer layout

["Kyrie Wu (吴晗)" <Kyrie.Wu@mediatek.com>]

On Fri, 2025-05-30 at 13:38 -0400, Nicolas Dufresne wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi,
> 
> Le vendredi 30 mai 2025 à 15:45 +0800, Kyrie Wu a écrit :
> > 1. change dst buffer size to same as struct mtk_jpeg_src_buf
> > to make sure all params of mtk_jpeg_src_buf could get a memory.
> > 2. For memory alloc operation:
> > the v4l2 framework malloc a memory, the base addr is vb2_buffer and
> > the size is sizeof(struct mtk_jpeg_src_buf), mtk_jpeg_src_buf could
> > get
> > itself addr by container_of like that:
> > vb2_buffer -> vb2_v4l2_buffer -> mtk_jpeg_src_buf.
> > vb2_v4l2_buffer must keep on the top of mtk_jpeg_src_buf.
> 
> The subject imply a refactoring, but the most important change in
> your
> patch is to fix the wrong buf strut size. Can you rework the subject
> and message to state what you are fixing please.
> 
> Add a Fixes tag, and moved it at the start of the series to show this
> isn't a problem you have introduced in previous patch.

Dear Nicolas,

Thanks for your suggestion and I will change the tag from "refactoring"
to "fixes" and add the tag.

Thanks.
> 
> > 
> > Signed-off-by: Kyrie Wu <kyrie.wu@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 52d59bb5c9ad..7e3509be6f69 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > @@ -1103,7 +1103,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 655dc9c3280c..186cd1862028 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > @@ -96,10 +96,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;
> 
> This "refactoring" should be split, it does not fix anything.

Thanks. I will change it in the coming version 6.
> 
> Nicolas

Regards,
Kyrie.
> 
> >       struct mtk_jpeg_dec_param dec_param;
> > 
> >       struct mtk_jpeg_ctx *curr_ctx;