linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jungo Lin <jungo.lin@mediatek.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: <tfiga@chromium.org>, <laurent.pinchart@ideasonboard.com>,
	<matthias.bgg@gmail.com>, <mchehab@kernel.org>,
	<shik@chromium.org>, <devicetree@vger.kernel.org>,
	<Sean.Cheng@mediatek.com>, <suleiman@chromium.org>,
	<Rynn.Wu@mediatek.com>, Pi-Hsun Shih <pihsun@chromium.org>,
	<srv_heupstream@mediatek.com>, <robh@kernel.org>,
	<ryan.yu@mediatek.com>, <Jerry-ch.Chen@mediatek.com>,
	<frankie.chiu@mediatek.com>, <sj.huang@mediatek.com>,
	<yuzhao@chromium.org>, <linux-mediatek@lists.infradead.org>,
	<zwisler@chromium.org>, <ddavenport@chromium.org>,
	<frederic.chen@mediatek.com>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-media@vger.kernel.org>
Subject: Re: [v6, 5/5] media: platform: Add Mediatek ISP P1 V4L2 device driver
Date: Tue, 28 Jan 2020 10:13:47 +0800	[thread overview]
Message-ID: <1580177627.13350.8.camel@mtksdccf07> (raw)
In-Reply-To: <c481734a-c706-2ea9-3888-ab2fb9c805ef@xs4all.nl>

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@mediatek.com>
> > Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > ---
> > 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


  reply	other threads:[~2020-01-28  2:13 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <Jungo Lin <jungo.lin@mediatek.com>
2019-04-02 10:04 ` [PATCH v1] media: media_device_enum_links32: fix missing reserved field copy Jungo Lin
2019-04-02 11:33   ` Laurent Pinchart
2019-04-03  0:30     ` Jungo Lin
2019-04-03  1:44 ` [PATCH] media: media_device_enum_links32: clean a reserved field Jungo Lin
2019-05-10  1:57 ` [RFC,V2,00/11] meida: platform: mtk-isp: Add Mediatek ISP Pass 1 driver Jungo Lin
2019-05-10  1:57 ` [RFC,V2,01/11] dt-bindings: mt8183: Add binding for ISP Pass 1 reserved memory Jungo Lin
2019-05-14 19:50   ` Rob Herring
2019-05-15 13:02     ` Jungo Lin
2019-05-10  1:57 ` [RFC,V2,02/11] dts: arm64: mt8183: Add ISP Pass 1 shared memory node Jungo Lin
2019-05-10  1:57 ` [RFC,V2,03/11] dt-bindings: mt8183: Added camera ISP Pass 1 Jungo Lin
2019-05-14 19:54   ` Rob Herring
2019-05-16  6:12     ` Jungo Lin
2019-05-10  1:57 ` [RFC,V2,04/11] dts: arm64: mt8183: Add ISP Pass 1 nodes Jungo Lin
2019-05-10  1:57 ` [RFC,V2,05/11] media: platform: Add Mediatek ISP Pass 1 driver Kconfig Jungo Lin
2019-05-10  1:57 ` [RFC,V2,06/11] media: platform: Add Mediatek ISP P1 image & meta formats Jungo Lin
2019-05-13  8:35   ` Hans Verkuil
2019-05-15 12:49     ` [RFC, V2, 06/11] " Jungo Lin
2019-05-10  1:58 ` [RFC,V2,07/11] media: platform: Add Mediatek ISP P1 private control Jungo Lin
2019-05-13  8:46   ` Hans Verkuil
2019-05-14  6:23     ` Jungo Lin
2019-10-02 10:55     ` Sakari Ailus
2019-10-02 11:02       ` Sakari Ailus
2019-05-10  1:58 ` [RFC,V2,08/11] media: platform: Add Mediatek ISP P1 V4L2 functions Jungo Lin
2019-05-24 18:49   ` Drew Davenport
2019-05-28  1:00     ` Jungo Lin
2019-05-10  1:58 ` [RFC,V2,09/11] media: platform: Add Mediatek ISP P1 device driver Jungo Lin
2019-05-24 21:19   ` Drew Davenport
2019-05-27 13:07     ` [RFC, V2, 09/11] " Jungo Lin
2019-05-10  1:58 ` [RFC,V2,10/11] media: platform: Add Mediatek ISP P1 SCP communication Jungo Lin
2019-05-10  1:58 ` [RFC,V2,11/11] media: platform: Add Mediatek ISP P1 shared memory device Jungo Lin
2019-08-07 12:47 ` [RFC,v4,0/4] media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver Jungo Lin
2019-08-07 12:48   ` [RFC,v4,1/4] media: dt-bindings: mt8183: Added camera ISP Pass 1 Jungo Lin
2019-08-21 19:47     ` Rob Herring
2019-08-22 12:47       ` Jungo Lin
2019-08-21 20:17     ` Rob Herring
2019-08-22 12:48       ` Jungo Lin
2019-08-07 12:48   ` [RFC,v4,2/4] dts: arm64: mt8183: Add ISP Pass 1 nodes Jungo Lin
2019-08-07 12:48   ` [RFC,v4,3/4] media: platform: Add Mediatek ISP P1 image & meta formats Jungo Lin
2019-08-07 12:48   ` [RFC,v4,4/4] media: platform: Add Mediatek ISP P1 V4L2 device driver Jungo Lin
2019-09-02  7:51 ` [RFC,v5,0/5] media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver Jungo Lin
2019-09-02  7:51   ` [RFC,v5, 1/5] media: dt-bindings: mt8183: Added camera ISP Pass 1 Jungo Lin
2019-09-02 15:17     ` Rob Herring
2019-09-02  7:51   ` [RFC,v5, 2/5] dts: arm64: mt8183: Add ISP Pass 1 nodes Jungo Lin
2019-09-02  7:51   ` [RFC,v5, 3/5] media: videodev2.h: Add new boottime timestamp type Jungo Lin
2019-09-02  7:51   ` [RFC,v5, 4/5] media: pixfmt: Add Mediatek ISP P1 image & meta formats Jungo Lin
2019-09-02  7:51   ` [RFC,v5, 5/5] media: platform: Add Mediatek ISP P1 V4L2 device driver Jungo Lin
2019-12-19  5:49 ` [v6, 0/5] media: media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver Jungo Lin
2019-12-19  5:49   ` [v6, 1/5] media: dt-bindings: mt8183: Added camera ISP Pass 1 Jungo Lin
2020-03-31 15:34     ` Helen Koike
2020-04-10 10:04       ` Jungo Lin
2019-12-19  5:49   ` [v6, 2/5] dts: arm64: mt8183: Add ISP Pass 1 nodes Jungo Lin
2019-12-19  5:49   ` [v6, 3/5] media: videodev2.h: Add new boottime timestamp type Jungo Lin
2020-01-07 14:10     ` Hans Verkuil
     [not found]       ` <e833b88ba74945c495a102c98cd54725@mtkmbs07n1.mediatek.inc>
2020-01-10  9:59         ` Jungo Lin
2020-01-10 10:08       ` Jungo Lin
2019-12-19  5:49   ` [v6, 4/5] media: platform: Add Mediatek ISP P1 image & meta formats Jungo Lin
2020-04-03  2:30     ` Laurent Pinchart
2020-04-10 10:00       ` Jungo Lin
2019-12-19  5:49   ` [v6, 5/5] media: platform: Add Mediatek ISP P1 V4L2 device driver Jungo Lin
2020-01-23 13:59     ` Hans Verkuil
2020-01-28  2:13       ` Jungo Lin [this message]
2020-03-31 15:34     ` Helen Koike
2020-04-09  2:05       ` Jungo Lin
2020-04-14 12:25         ` Helen Koike
     [not found]           ` <b2c30e560e9b4ec488957ca62bae09fe@mtkmbs01n2.mediatek.inc>
2020-05-04 12:27             ` Jungo Lin
2020-05-05 15:38               ` Helen Koike
2020-04-02 16:45     ` Dafna Hirschfeld
2020-04-09  2:49       ` Jungo Lin
2020-03-31 15:34   ` [v6, 0/5] media: media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver Helen Koike
2020-04-10 10:32     ` Jungo Lin
2020-04-14 12:25       ` Helen Koike
     [not found]         ` <1fd3615eb18f48ada186bfe228fc907b@mtkmbs01n2.mediatek.inc>
2020-05-04 12:40           ` Jungo Lin
2020-05-05 15:30             ` Helen Koike
2020-05-05 16:18               ` Tomasz Figa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1580177627.13350.8.camel@mtksdccf07 \
    --to=jungo.lin@mediatek.com \
    --cc=Jerry-ch.Chen@mediatek.com \
    --cc=Rynn.Wu@mediatek.com \
    --cc=Sean.Cheng@mediatek.com \
    --cc=ddavenport@chromium.org \
    --cc=devicetree@vger.kernel.org \
    --cc=frankie.chiu@mediatek.com \
    --cc=frederic.chen@mediatek.com \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=pihsun@chromium.org \
    --cc=robh@kernel.org \
    --cc=ryan.yu@mediatek.com \
    --cc=shik@chromium.org \
    --cc=sj.huang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    --cc=suleiman@chromium.org \
    --cc=tfiga@chromium.org \
    --cc=yuzhao@chromium.org \
    --cc=zwisler@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).