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();
> +}
> +
[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;
> +}
> +
