Hi, Hans:
On Thu, 2020-01-23 at 14:59 +0100, Hans Verkuil wrote:
> Hi Jungo,
>
> On 12/19/19 6:49 AM, Jungo Lin wrote:
> > This patch adds the Mediatek ISP P1 HW control device driver.
> > It handles the ISP HW configuration, provides interrupt handling and
> > initializes the V4L2 device nodes and other V4L2 functions. Moreover,
> > implement standard V4L2 video driver that utilizes V4L2 and media
> > framework APIs. It supports one media device, one sub-device and
> > several video devices during initialization. Moreover, it also connects
> > with sensor and seninf drivers with V4L2 async APIs. Communicate with
> > co-process via SCP communication to compose ISP registers in the
> > firmware.
> >
> > (The current metadata interface used in meta input and partial
> > meta nodes is only a temporary solution to kick off the driver
> > development and is not ready to be reviewed yet.)
> >
> > Signed-off-by: Jungo Lin <jungo.lin@xxxxxxxxxxxx>
> > Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > Signed-off-by: Pi-Hsun Shih <pihsun@xxxxxxxxxxxx>
> > ---
> > Changes from v6:
> > - Revise help description for VIDEO_MEDIATEK_ISP_PASS1
> > - Apply SCP v21 change in P1 driver by Pi-Hsun Shih
> > - Correct auto suspend timer value for suspend/resume issue
> > - Increase IPI guard timer to 1 second to avoid false alarm command timeout event
> > - Fix KE due to no sen-inf sub-device
> > ---
> > drivers/media/platform/mtk-isp/Kconfig | 20 +
> > .../media/platform/mtk-isp/isp_50/Makefile | 3 +
> > .../platform/mtk-isp/isp_50/cam/Makefile | 6 +
> > .../platform/mtk-isp/isp_50/cam/mtk_cam-hw.c | 636 +++++
> > .../platform/mtk-isp/isp_50/cam/mtk_cam-hw.h | 64 +
> > .../platform/mtk-isp/isp_50/cam/mtk_cam-ipi.h | 222 ++
> > .../mtk-isp/isp_50/cam/mtk_cam-regs.h | 95 +
> > .../platform/mtk-isp/isp_50/cam/mtk_cam.c | 2087 +++++++++++++++++
> > .../platform/mtk-isp/isp_50/cam/mtk_cam.h | 244 ++
> > 9 files changed, 3377 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/Kconfig
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/Makefile
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-hw.c
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-hw.h
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ipi.h
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
>
> <snip>
>
> > +static void isp_tx_frame_worker(struct work_struct *work)
> > +{
> > + struct mtk_cam_dev_request *req =
> > + container_of(work, struct mtk_cam_dev_request, frame_work);
> > + struct mtk_cam_dev *cam =
> > + container_of(req->req.mdev, struct mtk_cam_dev, media_dev);
> > + struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
> > +
> > + scp_ipi_send(p1_dev->scp, SCP_IPI_ISP_FRAME, &req->frame_params,
> > + sizeof(req->frame_params), MTK_ISP_IPI_SEND_TIMEOUT);
> > +}
>
> <snip>
>
> > +void mtk_isp_req_enqueue(struct mtk_cam_dev *cam,
> > + struct mtk_cam_dev_request *req)
> > +{
> > + struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
> > +
> > + /* Accumulated frame sequence number */
> > + req->frame_params.frame_seq_no = ++p1_dev->enqueued_frame_seq_no;
> > +
> > + INIT_WORK(&req->frame_work, isp_tx_frame_worker);
> > + queue_work(p1_dev->composer_wq, &req->frame_work);
> > + dev_dbg(cam->dev, "enqueue fd:%s frame_seq_no:%d job cnt:%d\n",
> > + req->req.debug_str, req->frame_params.frame_seq_no,
> > + cam->running_job_count);
> > +}
>
> <snip>
>
> > +/*
> > + * struct dma_buffer - DMA buffer address information
> > + *
> > + * @iova: DMA address for ISP DMA device
> > + * @scp_addr: SCP address for external co-process unit
> > + *
> > + */
> > +struct dma_buffer {
> > + u32 iova;
> > + u32 scp_addr;
> > +} __packed;
>
> <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 = mtk_cam_vb2_buf_to_dev_buf(vb);
> > + struct mtk_cam_dev_request *req = mtk_cam_req_to_dev_req(vb->request);
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> > + struct device *dev = cam->dev;
> > + unsigned long flags;
> > +
> > + dev_dbg(dev, "%s: node:%d fd:%d idx:%d\n", __func__,
> > + node->id, buf->vbb.request_fd, buf->vbb.vb2_buf.index);
> > +
> > + /* added the buffer into the tracking list */
> > + spin_lock_irqsave(&node->buf_list_lock, flags);
> > + list_add_tail(&buf->list, &node->buf_list);
> > + spin_unlock_irqrestore(&node->buf_list_lock, flags);
> > +
> > + /* update buffer internal address */
> > + req->frame_params.dma_bufs[buf->node_id].iova = buf->daddr;
> > + req->frame_params.dma_bufs[buf->node_id].scp_addr = buf->scp_addr;
> > +}
> > +
>
> <snip>
>
> > +/*
> > + * struct mtk_p1_frame_param - MTK ISP P1 driver frame parameters.
> > + *
> > + * @frame_seq_no: The frame sequence of frame in driver layer.
> > + * @dma_bufs: The DMA buffer address information of enabled DMA nodes.
> > + *
> > + */
> > +struct mtk_p1_frame_param {
> > + unsigned int frame_seq_no;
> > + struct dma_buffer dma_bufs[MTK_CAM_P1_TOTAL_NODES];
> > +} __packed;
>
> So if I understand this correctly, to set the ISP frame parameters userspace
> provides an array of pointers to other memory areas that are magically created
> somewhere and containing magic, undocumented data.
>
> I know you said that this is 'not ready to be reviewed yet', but I just wanted
> to mention that this is of course not acceptable and needs to be replaced with
> a documented metadata structure that userspace can pass in the metadata buffer.
>
> Just ignore this email if you were already planning on doing that. I just wanted
> to make sure that it is clear that the current approach won't fly.
>
> Regards,
>
> Hans
>
Thanks for your comment.
Firstly, I think I miss meta data types definition in this series.
https://patchwork.kernel.org/patch/11126055/
include/uapi/linux/videodev2.h
+#define V4L2_META_FMT_MTISP_3A v4l2_fourcc('M', 'T', 'f', 'a') /*
AE/AWB histogram */
+#define V4L2_META_FMT_MTISP_AF v4l2_fourcc('M', 'T', 'f', 'f') /* AF
histogram */
+#define V4L2_META_FMT_MTISP_LCS v4l2_fourcc('M', 'T', 'f', 'c') /*
Local contrast enhanced statistics */
+#define V4L2_META_FMT_MTISP_LMV v4l2_fourcc('M', 'T', 'f', 'm') /*
Local motion vector histogram */
+#define V4L2_META_FMT_MTISP_PARAMS v4l2_fourcc('M', 'T', 'f', 'p') /*
ISP tuning parameters */
We will correct this missing error in next patch set.
Secondly, we are working on the documented meta-data structures for
these meta nodes, especially on 4L2_META_FMT_MTISP_PARAMS which is used
for tuning parameters from user space.
Sincerely
Jungo
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
