Hi Julien, Thank you for the patch. A first review comment, for an issue I've noticed while testing. On Mon, Aug 07, 2023 at 11:48:13AM +0200, Julien Stephan wrote: > From: Phi-bang Nguyen <pnguyen@xxxxxxxxxxxx> > > 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@xxxxxxxxxxxx> > Signed-off-by: Florian Sylvestre <fsylvestre@xxxxxxxxxxxx> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx> > --- > MAINTAINERS | 1 + > .../platform/mediatek/isp/isp_30/Kconfig | 19 + > .../platform/mediatek/isp/isp_30/Makefile | 1 + > .../mediatek/isp/isp_30/camsv/Makefile | 7 + > .../mediatek/isp/isp_30/camsv/mtk_camsv.c | 328 ++++++++ > .../mediatek/isp/isp_30/camsv/mtk_camsv.h | 196 +++++ > .../isp/isp_30/camsv/mtk_camsv30_hw.c | 432 ++++++++++ > .../isp/isp_30/camsv/mtk_camsv30_regs.h | 60 ++ > .../isp/isp_30/camsv/mtk_camsv_video.c | 781 ++++++++++++++++++ > 9 files changed, 1825 insertions(+) > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_regs.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c [snip] > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > new file mode 100644 > index 000000000000..bdf878460354 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > @@ -0,0 +1,432 @@ [snip] > +static void mtk_camsv30_setup(struct mtk_cam_dev *cam_dev, u32 w, u32 h, > + u32 bpl, u32 mbus_fmt) > +{ > + const struct mtk_cam_conf *conf = cam_dev->conf; > + u32 int_en = INT_ST_MASK_CAMSV; > + u32 tmp; > + struct mtk_cam_sparams sparams; > + > + fmt_to_sparams(mbus_fmt, &sparams); > + > + spin_lock(&cam_dev->irqlock); This isn't right, for multiple reasons. First, the lock is taken in IRQ context, so you need to disable IRQs here, or you'll risk deadlocking. spin_lock_irqsave() is the safe wildcard solution, but if this function is guaranteed to be called with interrupts enabled, you can also use spin_lock_irq(). This problem is caught by lockdep, didn't you test the driver with lockdep enabled ? If not, please enable it for v4. Then, pm_runtime_get_sync() may sleep, so you should take the lock after. Next, mtk_camsv30_setup() is called by mtk_camsv30_runtime_resume() below, right after calling spin_lock_irqsave() on the same lock. This has quite clearly not been tested. Please make sure to test system suspend/resume while streaming for v4. Finally, does this function really require taking the irqlock ? It looks like locking should be revisited in this driver. > + > + if (pm_runtime_get_sync(cam_dev->dev) < 0) { When pm_runtime_get_sync() fails you need to call pm_runtime_put(). A better option is to use pm_runtime_resume_and_get(). > + dev_err(cam_dev->dev, "failed to get pm_runtime\n"); > + spin_unlock(&cam_dev->irqlock); > + return; > + } > + > + writel(conf->tg_sen_mode, cam_dev->regs_tg + CAMSV_TG_SEN_MODE); > + > + writel((w * sparams.w_factor) << 16U, cam_dev->regs_tg + CAMSV_TG_SEN_GRAB_PXL); > + > + writel(h << 16U, cam_dev->regs_tg + CAMSV_TG_SEN_GRAB_LIN); > + > + /* YUV_U2S_DIS: disable YUV sensor unsigned to signed */ > + writel(0x1000U, cam_dev->regs_tg + CAMSV_TG_PATH_CFG); > + > + /* Reset cam */ > + writel(CAMSV_SW_RST, cam_dev->regs + CAMSV_SW_CTL); > + writel(0x0U, cam_dev->regs + CAMSV_SW_CTL); > + writel(CAMSV_IMGO_RST_TRIG, cam_dev->regs + CAMSV_SW_CTL); > + > + readl_poll_timeout(cam_dev->regs + CAMSV_SW_CTL, tmp, > + (tmp == (CAMSV_IMGO_RST_TRIG | CAMSV_IMGO_RST_ST)), 10, 200); > + > + writel(0x0U, cam_dev->regs + CAMSV_SW_CTL); > + > + writel(int_en, cam_dev->regs + CAMSV_INT_EN); > + > + writel(conf->module_en | sparams.module_en_pak, > + cam_dev->regs + CAMSV_MODULE_EN); > + writel(sparams.fmt_sel, cam_dev->regs + CAMSV_FMT_SEL); > + writel(sparams.pak, cam_dev->regs + CAMSV_PAK); > + > + writel(bpl - 1U, cam_dev->regs_img0 + CAMSV_IMGO_SV_XSIZE); > + writel(h - 1U, cam_dev->regs_img0 + CAMSV_IMGO_SV_YSIZE); > + > + writel(sparams.imgo_stride | bpl, cam_dev->regs_img0 + CAMSV_IMGO_SV_STRIDE); > + > + writel(conf->imgo_con, cam_dev->regs_img0 + CAMSV_IMGO_SV_CON); > + writel(conf->imgo_con2, cam_dev->regs_img0 + CAMSV_IMGO_SV_CON2); > + > + /* CMOS_EN first */ > + writel(readl(cam_dev->regs_tg + CAMSV_TG_SEN_MODE) | CAMSV_TG_SEN_MODE_CMOS_EN, > + cam_dev->regs_tg + CAMSV_TG_SEN_MODE); > + > + /* finally, CAMSV_MODULE_EN : IMGO_EN */ > + writel(readl(cam_dev->regs + CAMSV_MODULE_EN) | CAMSV_MODULE_EN_IMGO_EN, > + cam_dev->regs + CAMSV_MODULE_EN); > + > + pm_runtime_put_autosuspend(cam_dev->dev); This will cut the power off after a delay, possibly loosing state. If userspace requests buffers and then starts streaming only later, you risk issues. > + spin_unlock(&cam_dev->irqlock); > +} > + > +static irqreturn_t isp_irq_camsv30(int irq, void *data) > +{ > + struct mtk_cam_dev *cam_dev = (struct mtk_cam_dev *)data; > + struct mtk_cam_dev_buffer *buf; > + unsigned long flags = 0; > + unsigned int irq_status; > + > + spin_lock_irqsave(&cam_dev->irqlock, flags); > + > + irq_status = readl(cam_dev->regs + CAMSV_INT_STATUS); > + > + if (irq_status & INT_ST_MASK_CAMSV_ERR) { > + dev_err(cam_dev->dev, "irq error 0x%x\n", > + (unsigned int)(irq_status & INT_ST_MASK_CAMSV_ERR)); > + } > + > + /* De-queue frame */ > + if (irq_status & CAMSV_IRQ_PASS1_DON) { > + cam_dev->sequence++; > + > + if (!cam_dev->is_dummy_used) { > + buf = list_first_entry_or_null(&cam_dev->buf_list, > + struct mtk_cam_dev_buffer, > + list); > + if (buf) { > + buf->v4l2_buf.sequence = cam_dev->sequence; > + buf->v4l2_buf.vb2_buf.timestamp = ktime_get_ns(); > + vb2_buffer_done(&buf->v4l2_buf.vb2_buf, > + VB2_BUF_STATE_DONE); > + list_del(&buf->list); > + } > + } > + > + if (list_empty(&cam_dev->buf_list)) { > + mtk_camsv30_update_buffers_add(cam_dev, &cam_dev->dummy); > + cam_dev->is_dummy_used = true; > + } else { > + buf = list_first_entry_or_null(&cam_dev->buf_list, > + struct mtk_cam_dev_buffer, > + list); > + mtk_camsv30_update_buffers_add(cam_dev, buf); > + cam_dev->is_dummy_used = false; > + } > + } > + > + spin_unlock_irqrestore(&cam_dev->irqlock, flags); > + > + return IRQ_HANDLED; > +} > + > +static int mtk_camsv30_runtime_suspend(struct device *dev) > +{ > + struct mtk_cam_dev *cam_dev = dev_get_drvdata(dev); > + struct vb2_queue *vbq = &cam_dev->vdev.vbq; > + > + if (vb2_is_streaming(vbq)) { > + mutex_lock(&cam_dev->op_lock); > + v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 0); > + mutex_unlock(&cam_dev->op_lock); > + } > + > + clk_bulk_disable_unprepare(cam_dev->num_clks, cam_dev->clks); > + > + return 0; > +} > + > +static int mtk_camsv30_runtime_resume(struct device *dev) > +{ > + struct mtk_cam_dev *cam_dev = dev_get_drvdata(dev); > + struct mtk_cam_video_device *vdev = &cam_dev->vdev; > + const struct v4l2_pix_format_mplane *fmt = &vdev->format; > + struct vb2_queue *vbq = &vdev->vbq; > + struct mtk_cam_dev_buffer *buf, *buf_prev; > + int ret; > + unsigned long flags = 0; > + > + ret = clk_bulk_prepare_enable(cam_dev->num_clks, cam_dev->clks); > + if (ret) { > + dev_err(dev, "failed to enable clock:%d\n", ret); > + return ret; > + } > + > + if (vb2_is_streaming(vbq)) { > + spin_lock_irqsave(&cam_dev->irqlock, flags); > + > + mtk_camsv30_setup(cam_dev, fmt->width, fmt->height, > + fmt->plane_fmt[0].bytesperline, vdev->fmtinfo->code); > + > + buf = list_first_entry_or_null(&cam_dev->buf_list, > + struct mtk_cam_dev_buffer, > + list); > + if (buf) { > + mtk_camsv30_update_buffers_add(cam_dev, buf); > + cam_dev->is_dummy_used = false; > + } else { > + mtk_camsv30_update_buffers_add(cam_dev, &cam_dev->dummy); > + cam_dev->is_dummy_used = true; > + } > + > + mtk_camsv30_cmos_vf_hw_enable(cam_dev, vdev->fmtinfo->packed); > + > + spin_unlock_irqrestore(&cam_dev->irqlock, flags); > + > + /* Stream on the sub-device */ > + mutex_lock(&cam_dev->op_lock); > + ret = v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 1); > + > + if (ret) { > + cam_dev->stream_count--; > + if (cam_dev->stream_count == 0) > + media_pipeline_stop(vdev->vdev.entity.pads); > + } > + mutex_unlock(&cam_dev->op_lock); > + > + if (ret) > + goto fail_no_stream; > + } > + > + return 0; > + > +fail_no_stream: > + spin_lock_irqsave(&cam_dev->irqlock, flags); > + list_for_each_entry_safe(buf, buf_prev, &cam_dev->buf_list, list) { > + buf->daddr = 0ULL; > + list_del(&buf->list); > + vb2_buffer_done(&buf->v4l2_buf.vb2_buf, VB2_BUF_STATE_ERROR); > + } > + spin_unlock_irqrestore(&cam_dev->irqlock, flags); > + return ret; > +} [snip] > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c > new file mode 100644 > index 000000000000..f879726eacd8 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c > @@ -0,0 +1,781 @@ [snip] > +/* ----------------------------------------------------------------------------- > + * VB2 Queue Operations > + */ > + > +static int mtk_cam_vb2_queue_setup(struct vb2_queue *vq, > + unsigned int *num_buffers, > + unsigned int *num_planes, > + unsigned int sizes[], > + struct device *alloc_devs[]) > +{ > + struct mtk_cam_video_device *vdev = > + vb2_queue_to_mtk_cam_video_device(vq); > + unsigned int max_buffer_count = vdev->desc->max_buf_count; > + const struct v4l2_pix_format_mplane *fmt = &vdev->format; > + struct mtk_cam_dev *cam = vb2_get_drv_priv(vq); > + unsigned int size; > + unsigned int np_conf; > + unsigned int i; > + > + /* Check the limitation of buffer size */ > + if (max_buffer_count) > + *num_buffers = clamp_val(*num_buffers, 1, max_buffer_count); > + > + size = fmt->plane_fmt[0].sizeimage; > + /* Add for q.create_bufs with fmt.g_sizeimage(p) / 2 test */ > + > + np_conf = cam->conf->frm_hdr_en ? 2 : 1; > + > + if (*num_planes == 0) { > + *num_planes = np_conf; > + for (i = 0; i < *num_planes; ++i) > + sizes[i] = size; > + } else if (*num_planes != np_conf || sizes[0] < size) { > + return -EINVAL; > + } > + > + (*cam->hw_functions->mtk_cam_setup)(cam, fmt->width, fmt->height, > + fmt->plane_fmt[0].bytesperline, vdev->fmtinfo->code); This isn't the right time to call this. The hardware should be programmed when starting streaming, not when allocating buffers. > + > + return 0; > +} [snip] -- Regards, Laurent Pinchart