linux-kernel - Re: [PATCH v5 4/5] media: platform: mediatek: isp

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240729141459.GA1552@pendragon.ideasonboard.com>
Date: Mon, 29 Jul 2024 17:14:59 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Julien Stephan <jstephan@...libre.com>
Cc: CK Hu (胡俊光) <ck.hu@...iatek.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>,
	Andy Hsieh (謝智皓) <Andy.Hsieh@...iatek.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"angelogioacchino.delregno@...labora.com" <angelogioacchino.delregno@...labora.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"paul.elder@...asonboard.com" <paul.elder@...asonboard.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"fsylvestre@...libre.com" <fsylvestre@...libre.com>,
	"pnguyen@...libre.com" <pnguyen@...libre.com>
Subject: Re: [PATCH v5 4/5] media: platform: mediatek: isp_30: add mediatek
 ISP3.0 camsv

On Mon, Jul 29, 2024 at 03:40:09PM +0200, Julien Stephan wrote:
> Le jeu. 18 juil. 2024 à 04:54, CK Hu (胡俊光) <ck.hu@...iatek.com> a écrit :
> >
> > Hi, Julien:
> >
> > On Thu, 2024-07-04 at 15:36 +0200, 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 int mtk_cam_vb2_start_streaming(struct vb2_queue *vq,
> > > +       unsigned int count)
> > > +{
> > > +struct mtk_cam_dev *cam = vb2_get_drv_priv(vq);
> > > +struct mtk_cam_dev_buffer *buf;
> > > +struct mtk_cam_video_device *vdev =
> > > +vb2_queue_to_mtk_cam_video_device(vq);
> > > +struct device *dev = cam->dev;
> > > +const struct v4l2_pix_format_mplane *fmt = &vdev->format;
> > > +int ret;
> > > +unsigned long flags;
> > > +
> > > +if (pm_runtime_get_sync(dev) < 0) {
> > > +dev_err(dev, "failed to get pm_runtime\n");
> > > +pm_runtime_put_autosuspend(dev);
> > > +return -1;
> > > +}
> > > +
> > > +(*cam->hw_functions->mtk_cam_setup)(cam, fmt->width, fmt->height,
> > > +fmt->plane_fmt[0].bytesperline, vdev->fmtinfo->code);
> > > +
> > > +
> > > +/* Enable CMOS and VF */
> > > +mtk_cam_cmos_vf_enable(cam, true, true);
> > > +
> > > +mutex_lock(&cam->op_lock);
> > > +
> > > +ret = mtk_cam_verify_format(cam);
> > > +if (ret < 0)
> > > +goto fail_unlock;
> > > +
> > > +/* Start streaming of the whole pipeline now*/
> > > +if (!cam->pipeline.start_count) {
> > > +ret = media_pipeline_start(vdev->vdev.entity.pads,
> > > +   &cam->pipeline);
> > > +if (ret) {
> > > +dev_err(dev, "failed to start pipeline:%d\n", ret);
> > > +goto fail_unlock;
> > > +}
> > > +}
> > > +
> > > +/* Media links are fixed after media_pipeline_start */
> > > +cam->stream_count++;
> > > +
> > > +cam->sequence = (unsigned int)-1;
> > > +
> > > +/* Stream on the sub-device */
> > > +ret = v4l2_subdev_call(&cam->subdev, video, s_stream, 1);
> > > +if (ret)
> > > +goto fail_no_stream;
> > > +
> > > +mutex_unlock(&cam->op_lock);
> > > +
> > > +/* Create dummy buffer */
> > > +cam->dummy_size = fmt->plane_fmt[0].sizeimage;
> > > +
> > > +cam->dummy.vaddr = dma_alloc_coherent(cam->dev, cam->dummy_size,
> > > +      &cam->dummy.daddr, GFP_KERNEL);
> >
> > Dummy buffer cost much in DRAM footprint. I think we can get rid of
> > this dummy buffer. If no buffer is queued from user space, call
> > mtk_camsv30_cmos_vf_hw_disable() to stop write data into DRAM. After
> > buffer is queued from user space, call mtk_camsv30_cmos_vf_hw_enable()
> > to start write data into DRAM.
> 
> Hi CK,
> 
> IMHO it does not cost that much. A long time ago, we tried to remove
> it, but we faced an issue (can't remember what :/).
> Moreover, some other driver already uses the dummy buffer
> implementation, if I am not wrong.

The hardware have a CAMSV_IMGO_SV_STRIDE register. What happens if you
set the stride to 0 instead of bytesperline ? Will the hardware write
repeatedly over the same line ? If so you can allocate a scratch buffer
of a single line.

You will need to ensure that, whenever you reconfigure the device, the
stride and buffer address will always be programmed atomically. If you
switch to the line buffer and the image starts before the stride is
reconfigure, bad things will happen.

Stopping the DMA is another solution that would I think be even better
if that can be done quickly (without waiting synchronously for the end
of the next frame), and if restarting is equally quick.

> > > +if (!cam->dummy.vaddr) {
> > > +ret = -ENOMEM;
> > > +goto fail_no_buffer;
> > > +}
> > > +
> > > +/* update first buffer address */
> > > +
> > > +/* added the buffer into the tracking list */
> > > +spin_lock_irqsave(&cam->irqlock, flags);
> > > +if (list_empty(&cam->buf_list)) {
> > > +(*cam->hw_functions->mtk_cam_update_buffers_add)(cam, &cam->dummy);
> > > +cam->is_dummy_used = true;
> > > +} else {
> > > +buf = list_first_entry_or_null(&cam->buf_list,
> > > +       struct mtk_cam_dev_buffer,
> > > +       list);
> > > +(*cam->hw_functions->mtk_cam_update_buffers_add)(cam, buf);
> > > +cam->is_dummy_used = false;
> > > +}
> > > +spin_unlock_irqrestore(&cam->irqlock, flags);
> > > +
> > > +return 0;
> > > +
> > > +fail_no_buffer:
> > > +mutex_lock(&cam->op_lock);
> > > +v4l2_subdev_call(&cam->subdev, video, s_stream, 0);
> > > +fail_no_stream:
> > > +cam->stream_count--;
> > > +if (cam->stream_count == 0)
> > > +media_pipeline_stop(vdev->vdev.entity.pads);
> > > +fail_unlock:
> > > +mutex_unlock(&cam->op_lock);
> > > +mtk_cam_vb2_return_all_buffers(cam, VB2_BUF_STATE_QUEUED);
> > > +
> > > +return ret;
> > > +}
> > > +

-- 
Regards,

Laurent Pinchart