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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9e9fd35b62cb1c9e234be0ff432aa4a965801e08.camel@mediatek.com>
Date: Wed, 9 Apr 2025 02:40:55 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: Julien Stephan <jstephan@...libre.com>
CC: "robh@...nel.org" <robh@...nel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "paul.elder@...asonboard.com"
	<paul.elder@...asonboard.com>, "laurent.pinchart@...asonboard.com"
	<laurent.pinchart@...asonboard.com>, "mchehab@...nel.org"
	<mchehab@...nel.org>, Andy Hsieh (謝智皓)
	<Andy.Hsieh@...iatek.com>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"fsylvestre@...libre.com" <fsylvestre@...libre.com>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "pnguyen@...libre.com" <pnguyen@...libre.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>
Subject: Re: [PATCH v8 4/5] media: platform: mediatek: isp: add mediatek
 ISP3.0 camsv

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@...iatek.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@...libre.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@...libre.com>
> > > Signed-off-by: Florian Sylvestre <fsylvestre@...libre.com>
> > > [Paul Elder fix irq locking]
> > > Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> > > Co-developed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > Co-developed-by: Julien Stephan <jstephan@...libre.com>
> > > Signed-off-by: Julien Stephan <jstephan@...libre.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
> 
> > > +}
> > > +
> >