Re: [PATCH v8 4/5] media: platform: mediatek: isp: add mediatek ISP3.0 camsv

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Tue, 2025-04-08 at 15:56 +0200, Julien Stephan wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Le jeu. 20 mars 2025 à 03:18, CK Hu (胡俊光) <ck.hu@mediatek.com> a écrit :
> > 
> > On Wed, 2025-01-22 at 14:59 +0100, Julien Stephan wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > From: Phi-bang Nguyen <pnguyen@baylibre.com>
> > > 
> > > This driver provides a path to bypass the SoC ISP so that image data
> > > coming from the SENINF can go directly into memory without any image
> > > processing. This allows the use of an external ISP.
> > > 
> > > Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > [Paul Elder fix irq locking]
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > > ---
> > 

[snip]

> > 
> > > +
> > > +static void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb)
> > > +{
> > > +       struct mtk_cam_dev *cam = vb2_get_drv_priv(vb->vb2_queue);
> > > +       struct mtk_cam_dev_buffer *buf = to_mtk_cam_dev_buffer(vb);
> > > +       unsigned long flags;
> > > +
> > > +       /* Add the buffer into the tracking list */
> > > +       spin_lock_irqsave(&cam->buf_list_lock, flags);
> > > +       if (vb2_start_streaming_called(vb->vb2_queue) && list_empty(&cam->buf_list))
> > > +               mtk_camsv_update_buffers_add(cam, buf);
> > > +
> > > +       list_add_tail(&buf->list, &cam->buf_list);
> > > +       spin_unlock_irqrestore(&cam->buf_list_lock, flags);
> > > +       if (vb2_start_streaming_called(vb->vb2_queue))
> > > +               mtk_camsv_fbc_inc(cam);
> > 
> > I think you should call mtk_camsv_fbc_inc() just after mtk_camsv_update_buffers_add();
> > 
> 
> Hi CK,
> 
> Is there any particular reason? I moved it at the bottom, to reduce
> the spinlock region as you requested in v7.. OR maybe I am missing
> something ?

I think I just partially review before, and this case is a little more complicated.
To let things be more simple,
I suggest to remove mtk_camsv_update_buffers_add() and mtk_camsv_fbc_inc() in mtk_cam_vb2_buf_queue(),
and do mtk_camsv_update_buffers_add() and mtk_camsv_fbc_inc() in irq handler and call mtk_camsv_fbc_inc() just after mtk_camsv_update_buffers_add().
So this spinlock just protect cam->buf_list and not protect register writing.
Call mtk_camsv_update_buffers_add() in mtk_cam_vb2_buf_queue() is too early because mtk_cam_vb2_buf_queue() is executed when scan line is in center of a video.
Hardware still need to wait for vblank to start writing a full video data.
So call mtk_camsv_update_buffers_add() and mtk_camsv_fbc_inc() only in irq handler would be reasonable and code flow would be simple.

Regards,
CK

> 
> Cheers
> Julien
> 
> > > +}
> > > +
> >