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

From: Julien Stephan
Date: Tue Apr 08 2025 - 10:01:56 EST


Le jeu. 20 mars 2025 à 03:18, CK Hu (胡俊光) <ck.hu@xxxxxxxxxxxx> 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@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>
> > [Paul Elder fix irq locking]
> > Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> > Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> > ---
>
> [snip]
>
> > +
> > +void mtk_camsv_setup(struct mtk_cam_dev *cam_dev, unsigned int count,
> > + u32 w, u32 h, u32 bpl, u32 mbus_fmt)
> > +{
> > + const struct mtk_cam_conf *conf = cam_dev->conf;
> > + const struct mtk_cam_format_info *fmtinfo;
> > + struct mtk_cam_dev_buffer *buf;
> > + unsigned long flags;
> > + unsigned int i;
> > + u32 tmp;
> > +
> > + for (i = 0; i < count; i++)
> > + mtk_camsv_fbc_inc(cam_dev);
>
> I think you should call mtk_camsv_fbc_inc() only after mtk_camsv_update_buffers_add().
>
> > +
> > + fmtinfo = mtk_cam_format_info_by_code(mbus_fmt);
> > +
> > + mtk_camsv_tg_write(cam_dev, CAMSV_TG_SEN_MODE, conf->tg_sen_mode);
> > +
> > + mtk_camsv_tg_write(cam_dev, CAMSV_TG_SEN_GRAB_PXL,
> > + (w * fmtinfo->w_factor) << 16U);
> > +
> > + mtk_camsv_tg_write(cam_dev, CAMSV_TG_SEN_GRAB_LIN, h << 16U);
> > +
> > + /* YUV_U2S_DIS: disable YUV sensor unsigned to signed */
> > + mtk_camsv_tg_write(cam_dev, CAMSV_TG_PATH_CFG, 0x1000U);
> > +
> > + /* Reset cam */
> > + mtk_camsv_write(cam_dev, CAMSV_SW_CTL, CAMSV_SW_RST);
> > + mtk_camsv_write(cam_dev, CAMSV_SW_CTL, 0x0U);
> > + mtk_camsv_write(cam_dev, CAMSV_SW_CTL, CAMSV_IMGO_RST_TRIG);
> > +
> > + readl_poll_timeout_atomic(cam_dev->regs + CAMSV_SW_CTL, tmp,
> > + (tmp == (CAMSV_IMGO_RST_TRIG |
> > + CAMSV_IMGO_RST_ST)), 10, 200);
> > +
> > + mtk_camsv_write(cam_dev, CAMSV_SW_CTL, 0x0U);
> > +
> > + mtk_camsv_write(cam_dev, CAMSV_INT_EN, INT_ST_MASK_CAMSV);
> > +
> > + mtk_camsv_write(cam_dev, CAMSV_MODULE_EN,
> > + conf->module_en | fmtinfo->module_en_pak);
> > + mtk_camsv_write(cam_dev, CAMSV_FMT_SEL, fmtinfo->fmt_sel);
> > + mtk_camsv_write(cam_dev, CAMSV_PAK, fmtinfo->pak);
> > +
> > + mtk_camsv_img0_write(cam_dev, CAMSV_IMGO_SV_XSIZE, bpl - 1U);
> > + mtk_camsv_img0_write(cam_dev, CAMSV_IMGO_SV_YSIZE, h - 1U);
> > +
> > + mtk_camsv_img0_write(cam_dev, CAMSV_IMGO_SV_STRIDE,
> > + fmtinfo->imgo_stride | bpl);
> > +
> > + mtk_camsv_img0_write(cam_dev, CAMSV_IMGO_SV_CON, conf->imgo_con);
> > + mtk_camsv_img0_write(cam_dev, CAMSV_IMGO_SV_CON2, conf->imgo_con2);
> > +
> > + /* Set buf addr */
> > + spin_lock_irqsave(&cam_dev->buf_list_lock, flags);
> > + buf = list_first_entry_or_null(&cam_dev->buf_list,
> > + struct mtk_cam_dev_buffer,
> > + list);
> > + if (buf)
> > + mtk_camsv_update_buffers_add(cam_dev, buf);
> > + spin_unlock_irqrestore(&cam_dev->buf_list_lock, flags);
> > +
> > + /* CMOS_EN first */
> > + mtk_camsv_tg_write(cam_dev, CAMSV_TG_SEN_MODE,
> > + mtk_camsv_tg_read(cam_dev, CAMSV_TG_SEN_MODE) |
> > + CAMSV_TG_SEN_MODE_CMOS_EN);
> > +
> > + /* finally, CAMSV_MODULE_EN : IMGO_EN */
> > + mtk_camsv_write(cam_dev, CAMSV_MODULE_EN,
> > + mtk_camsv_read(cam_dev, CAMSV_MODULE_EN) |
> > + CAMSV_MODULE_EN_IMGO_EN);
> > +}
> > +
>
> [snip]
>
> > +
> > +static int mtk_camsv_runtime_resume(struct device *dev)
> > +{
> > + struct mtk_cam_dev *cam_dev = dev_get_drvdata(dev);
> > + struct vb2_queue *vbq = &cam_dev->vdev.vbq;
> > + struct mtk_cam_dev_buffer *buf;
> > + unsigned long flags = 0;
> > + int ret;
> > +
> > + 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->buf_list_lock, flags);
> > + buf = list_first_entry_or_null(&cam_dev->buf_list,
> > + struct mtk_cam_dev_buffer,
> > + list);
> > + if (buf)
> > + mtk_camsv_update_buffers_add(cam_dev, buf);
>
> I don't know, when suspend, the register value would be kept or not.
> If register is not kept, I think you should re-program all register in mtk_camsv_setup().
> But it seems it's not necessary, so the register is kept and you don't need to set register again.
> So this mtk_camsv_update_buffers_add() is redundant.
>
> > +
> > + spin_unlock_irqrestore(&cam_dev->buf_list_lock, flags);
> > +
> > + /* Stream on the sub-device */
> > + ret = v4l2_subdev_enable_streams(&cam_dev->subdev,
> > + cam_dev->subdev_pads[MTK_CAM_CIO_PAD_VIDEO].index,
> > + BIT(0));
> > +
> > + if (ret)
> > + goto fail_no_stream;
> > + }
> > +
> > + return 0;
> > +
> > +fail_no_stream:
> > + mtk_cam_vb2_return_all_buffers(cam_dev, VB2_BUF_STATE_QUEUED);
> > + return ret;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_camsv_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > + pm_runtime_force_resume)
> > + SET_RUNTIME_PM_OPS(mtk_camsv_runtime_suspend,
> > + mtk_camsv_runtime_resume, NULL)
> > +};
> > +#endif
> > +
>
> [snip]
>
> > +/* -----------------------------------------------------------------------------
> > + * Format Information
> > + */
> > +
> > +static const struct mtk_cam_format_info mtk_cam_format_info[] = {
> > + {
> > + .fourcc = V4L2_PIX_FMT_SBGGR8,
> > + .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > + .bpp = 8,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_8
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_8,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGBRG8,
> > + .code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > + .bpp = 8,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_8
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_8,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGRBG8,
> > + .code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > + .bpp = 8,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_8
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_8,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SRGGB8,
> > + .code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > + .bpp = 8,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_8
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_8,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SBGGR10,
>
> In stream_out_fmts[], it does not declare V4L2_PIX_FMT_SBGGR10.
> So you may remove it in mtk_cam_format_info[], or add it in stream_out_fmts[].
>
> > + .code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > + .bpp = 10,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_10
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_10,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGBRG10,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > + .bpp = 10,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_10
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_10,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGRBG10,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > + .bpp = 10,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_10
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_10,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SRGGB10,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > + .bpp = 10,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_10
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_10,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SBGGR12,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > + .bpp = 12,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_12
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_12,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGBRG12,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > + .bpp = 12,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_12
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_12,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SGRBG12,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > + .bpp = 12,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_12
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_12,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_SRGGB12,
>
> Ditto.
>
> > + .code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > + .bpp = 12,
> > + .w_factor = 1,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_EN,
> > + .pak = CAMSV_PAK_PAK_MODE_RAW_12
> > + | CAMSV_PAK_PAK_DBL_MODE_2_PIXELS,
> > + .fmt_sel = CAMSV_FMT_SEL_TG1_FMT_RAW_12,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_32,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_UYVY,
> > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .bpp = 16,
> > + .w_factor = 2,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_SEL,
> > + .pak = 0, /* ignored */
> > + .fmt_sel = CAMSV_FMT_SEL_IMGO_BUS_SIZE_16
> > + | CAMSV_FMT_SEL_TG1_FMT_YUV422,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_16,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_VYUY,
> > + .code = MEDIA_BUS_FMT_VYUY8_1X16,
> > + .bpp = 16,
> > + .w_factor = 2,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_SEL,
> > + .pak = 0, /* ignored */
> > + .fmt_sel = CAMSV_FMT_SEL_IMGO_BUS_SIZE_16
> > + | CAMSV_FMT_SEL_TG1_FMT_YUV422,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_16,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_YUYV,
> > + .code = MEDIA_BUS_FMT_YUYV8_1X16,
> > + .bpp = 16,
> > + .w_factor = 2,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_SEL,
> > + .pak = 0, /* ignored */
> > + .fmt_sel = CAMSV_FMT_SEL_IMGO_BUS_SIZE_16
> > + | CAMSV_FMT_SEL_TG1_FMT_YUV422,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_16,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_YVYU,
> > + .code = MEDIA_BUS_FMT_YVYU8_1X16,
> > + .bpp = 16,
> > + .w_factor = 2,
> > + .module_en_pak = CAMSV_MODULE_EN_PAK_SEL,
> > + .pak = 0, /* ignored */
> > + .fmt_sel = CAMSV_FMT_SEL_IMGO_BUS_SIZE_16
> > + | CAMSV_FMT_SEL_TG1_FMT_YUV422,
> > + .imgo_stride = CAMSV_IMGO_SV_STRIDE_BUS_SIZE_EN
> > + | CAMSV_IMGO_SV_STRIDE_BUS_SIZE_16,
> > + },
> > +};
> > +
>
> [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 ?

Cheers
Julien

> > +}
> > +
>
> [snip]
>
> > +
> > +static int mtk_cam_vidioc_try_fmt(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_cam_video_device *vdev = file_to_mtk_cam_video_device(file);
> > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > + const struct mtk_cam_format_info *fmtinfo;
> > +
> > + /* Validate pixelformat */
> > + if (!mtk_cam_dev_find_fmt(vdev->desc, pix_mp->pixelformat))
> > + pix_mp->pixelformat = vdev->desc->fmts[0];
>
> In [1], it says return EINVAL when driver not support this buffer format.
>
> [1] https://docs.kernel.org/userspace-api/media/v4l/vidioc-g-fmt.html#description
>
> Regards,
> CK
>
> > +
> > + pix_mp->width = clamp_val(pix_mp->width, IMG_MIN_WIDTH, IMG_MAX_WIDTH);
> > + pix_mp->height = clamp_val(pix_mp->height, IMG_MIN_HEIGHT,
> > + IMG_MAX_HEIGHT);
> > +
> > + pix_mp->num_planes = 1;
> > +
> > + fmtinfo = mtk_cam_format_info_by_fourcc(pix_mp->pixelformat);
> > + calc_bpl_size_pix_mp(fmtinfo, pix_mp);
> > +
> > + /* Constant format fields */
> > + pix_mp->colorspace = V4L2_COLORSPACE_SRGB;
> > + pix_mp->field = V4L2_FIELD_NONE;
> > + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + pix_mp->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > +
> > + return 0;
> > +}
> > +
>
>
> ************* MEDIATEK Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!