Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
Hi Frederic,
On Tue, Jun 25, 2019 at 9:16 PM Frederic Chen
wrote:
>
> Dear Tomasz,
>
> Would you comment on the following points in further? Thank you for the
> review.
>
> On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote:
> > Hi Frederic,
> >
>
> [snip]
>
> > > +int mtk_dip_pipe_job_start(struct mtk_dip_pipe *dip_pipe,
> > > + struct mtk_dip_pipe_job_info *pipe_job_info)
> > > +{
> > > + struct platform_device *pdev = dip_pipe->dip_dev->pdev;
> > > + int ret;
> > > + int out_img_buf_idx;
> > > + struct img_ipi_frameparam dip_param;
> > > + struct mtk_dip_dev_buffer *dev_buf_in;
> > > + struct mtk_dip_dev_buffer *dev_buf_out;
> > > + struct mtk_dip_dev_buffer *dev_buf_tuning;
> > > +
> > > + if (!pipe_job_info) {
> > > + dev_err(>dev,
> > > + "pipe_job_info(%p) in start can't be NULL\n",
> > > + pipe_job_info);
> > > + return -EINVAL;
> > > + }
> >
> > This should be impossible to happen.
> >
> > > +
> > > + /* We need RAW and at least MDP0 or MDP 1 buffer */
> > > + if (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT] ||
> > > + (!pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE]
> > > &&
> > > +
> > > !pipe_job_info->buf_map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE])){
> > > + struct mtk_dip_dev_buffer **map = pipe_job_info->buf_map;
> > > +
> > > + dev_dbg(>dev,
> > > + "can't trigger job: raw(%p), mdp0(%p),
> > > mdp1(%p)\n",
> > > + map[MTK_DIP_VIDEO_NODE_ID_RAW_OUT],
> > > + map[MTK_DIP_VIDEO_NODE_ID_MDP0_CAPTURE],
> > > + map[MTK_DIP_VIDEO_NODE_ID_MDP1_CAPTURE]);
> > > + return -EINVAL;
> >
> > This must be validated at the time of request_validate. We can't fail at
> > this stage anymore.
>
> After the modification about checking the required buffers in
> req_validate(), we got failed in the following testRequests()
> of V4L2 compliance test. The V4L2 compliance test case doesn't know
> which buffers of the video devices are required and expects that the
> MEDIA_REQUEST_IOC_QUEUE succeed when the request has any valid buffer.
>
> For example, when the request has MDP 0 buffer only, the DIP's
> req_validate() should return an error since it also need a buffer
> from RAW video device, but it make compliance test get failed.
>
> May I still check the required buffers in req_validate() in the next
> patch? I will add some note to explain that the compliance test failed
> item is related to the limitation?
>
> ===
> int testRequests(struct node *node, bool test_streaming)
> // ..
> if (i)
> fail_on_test(!buf.qbuf(node));
> buf.s_flags(buf.g_flags() | V4L2_BUF_FLAG_REQUEST_FD);
> buf.s_request_fd(buf_req_fds[i]);
> buf.s_field(V4L2_FIELD_ANY);
> fail_on_test(buf.qbuf(node));
> if (v4l_type_is_video(buf.g_type()) && v4l_type_is_output(buf.g_type()))
> fail_on_test(buf.g_field() == V4L2_FIELD_ANY);
> fail_on_test(buf.querybuf(node, i));
>
> // ..
>
> // LINE 1807 in v4l2-test-buffers.cpp, we will got the failed here.
> // Since we need one RAW and one MDP0 buffer at least.
> // v4l2-test-buffers.cpp(1807): doioctl_fd(buf_req_fds[i],
> // MEDIA_REQUEST_IOC_QUEUE, 0)
> // test Requests: FAIL
> fail_on_test(doioctl_fd(buf_req_fds[i], MEDIA_REQUEST_IOC_QUEUE, 0));
> ===
>
Sounds like a limitation of the compliance test. Request API testing
there is still new and possibly just made for simple mem-to-mem
devices.
Hans, the driver always requires some buffers to be given, like the
raw frame input, while other, e.g. downscaled output, are optional.
Any ideas?
> > > +
> > > +static int mtk_dip_vb2_queue_setup(struct vb2_queue *vq,
> > > + unsigned int *num_buffers,
> > > + unsigned int *num_planes,
> > > + unsigned int sizes[],
> > > + struct device *alloc_devs[])
> > > +{
> > > + struct mtk_dip_pipe *dip_pipe = vb2_get_drv_priv(vq);
> > > + struct mtk_dip_video_device *node =
> > > + mtk_dip_vbq_to_node(vq);
> > > + struct device *dev = _pipe->dip_dev->pdev->dev;
> > > + struct device *buf_alloc_ctx;
> > > +
>
> [snip]
>
> > > +
> > > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE ||
> > > + vq->type == V4L2_BUF_TYPE_META_OUTPUT)
> > > + size = fmt->fmt.meta.buffersize;
> > > + else
> > > + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage;
> > > +
> > > + if (*num_planes) {
> > > + if (sizes[0] < size) {
> > > + dev_dbg(dev, "%s:%s:%s: size error(user:%d,
> > > max:%d)\n",
> > > +
Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
Hi Tomasz,
On Tue, 2019-06-11 at 19:13 +0900, Tomasz Figa wrote:
> On Tue, Jun 11, 2019 at 7:07 PM Frederic Chen
> wrote:
> >
> > Hi Tomasz,
> >
> >
> > On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote:
> > > On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen
> > > wrote:
> > > >
> > > > Dear Tomasz,
> > > >
> > > > I'd like to elaborate more about the tuning_data.va.
> > > > Would you like to give us some advice about our improvement proposal
> > > > inline?
> > > >
> > > > Thank you very much.
> > > >
> > > >
> > > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote:
> > > > > Dear Tomasz,
> > > > >
> > > > > I appreciate your comment. It is very helpful for us.
> > > > >
> > > > >
> > > > > > > diff --git
> > > > > > > a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > > b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > > new file mode 100644
> > > > > > > index ..54d2b5f5b802
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > > @@ -0,0 +1,1384 @@
> > > >
> > > > [snip]
> > > >
> > > > > > > +static void dip_submit_worker(struct work_struct *work)
> > > > > > > +{
> > > > > > > + struct mtk_dip_hw_submit_work *dip_submit_work =
> > > > > > > + container_of(work, struct mtk_dip_hw_submit_work,
> > > > > > > frame_work);
> > > > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw;
> > > > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw);
> > > > > > > + struct mtk_dip_hw_work *dip_work;
> > > > > > > + struct mtk_dip_hw_subframe *buf;
> > > > > > > + u32 len, num;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + num = atomic_read(_hw->num_composing);
> > > > > > > +
> > > > > > > + mutex_lock(_hw->dip_worklist.queuelock);
> > > > > > > + dip_work = list_first_entry(_hw->dip_worklist.queue,
> > > >
> > > > [snip]
> > > >
> > > > > > > +
> > > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > > > > + dev_dbg(_dev->pdev->dev,
> > > > > > > + "%s: frame_no(%d) has no tuning_data\n",
> > > > > > > + __func__, dip_work->frameparams.frame_no);
> > > > > > > +
> > > > > > > + memcpy(_work->frameparams.tuning_data,
> > > > > > > + >tuning_buf, sizeof(buf->tuning_buf));
> > > > > >
> > > > > > Ditto.
> > > > > >
> > > > >
> > > > > I got it.
> > > > >
> > > > > > > + memset((char *)buf->tuning_buf.va, 0,
> > > > > > > DIP_TUNING_SZ);
> > > > > >
> > > > > > Ditto.
> > > > >
> > > > > I got it.
> > > > >
> > > > > >
> > > > > > > + /*
> > > > > > > +* When user enqueued without tuning buffer,
> > > > > > > +* it would use driver internal buffer.
> > > > > > > +* So, tuning_data.va should be 0
> > > > > > > +*/
> > > > > > > + dip_work->frameparams.tuning_data.va = 0;
> > > > > >
> > > > > > I don't understand this. We just zeroed the buffer via this kernel
> > > > > > VA few
> > > > > > lines above, so why would it have to be set to 0?
> > > > > >
> > > > >
> > > > > I will remove this unnecessary line.
> > > > >
> > > > > > > + }
> > > >
> > > > After confirming the firmware part, I found that we use this field
> > > > (tuning_data.va) to notify firmware if there is no tuning data from
> > > > user.
> > > >
> > > > - frameparams.tuning_data.va is 0: use the default tuning data in
> > > >SCP, but we still need to pass
> > > >frameparams.tuning_data.pa because
> > > >the buffer contains some working
> > > >buffer required.
> > > > - frameparams.tuning_data.va is not 0: the tuning data was passed from
> > > >the user
> > > >
> > > > Since we should not pass cpu addres to SCP, could I rename
> > > > tuning_data.va
> > > > as tuning_data.cookie, and write a constant value to indicate if SCP
> > > > should use its internal default setting or not here?
> > > >
> > > > For example,
> > > > /* SCP uses tuning data passed from userspace*/
> > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA;
> > > >
> > > > /* SCP uses internal tuning data */
> > > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA;
> > >
> > > Perhaps we could just call it "present" and set to true or false?
> > >
> >
> > Yes. I would like to use "present" here.
> >
> > Original:
> > struct img_addr {
> > u64 va; /* Used by Linux OS access */
> > u32 pa; /* Used by CM4 access */
> > u32 iova; /* Used by IOMMU HW access */
> > } __attribute__ ((__packed__));
> >
> > struct img_ipi_frameparam {
> > u32 index;
> > u32 frame_no;
> > u64
Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
On Tue, Jun 11, 2019 at 7:07 PM Frederic Chen
wrote:
>
> Hi Tomasz,
>
>
> On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote:
> > On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen
> > wrote:
> > >
> > > Dear Tomasz,
> > >
> > > I'd like to elaborate more about the tuning_data.va.
> > > Would you like to give us some advice about our improvement proposal
> > > inline?
> > >
> > > Thank you very much.
> > >
> > >
> > > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote:
> > > > Dear Tomasz,
> > > >
> > > > I appreciate your comment. It is very helpful for us.
> > > >
> > > >
> > > > > > diff --git
> > > > > > a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > new file mode 100644
> > > > > > index ..54d2b5f5b802
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > > @@ -0,0 +1,1384 @@
> > >
> > > [snip]
> > >
> > > > > > +static void dip_submit_worker(struct work_struct *work)
> > > > > > +{
> > > > > > + struct mtk_dip_hw_submit_work *dip_submit_work =
> > > > > > + container_of(work, struct mtk_dip_hw_submit_work,
> > > > > > frame_work);
> > > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw;
> > > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw);
> > > > > > + struct mtk_dip_hw_work *dip_work;
> > > > > > + struct mtk_dip_hw_subframe *buf;
> > > > > > + u32 len, num;
> > > > > > + int ret;
> > > > > > +
> > > > > > + num = atomic_read(_hw->num_composing);
> > > > > > +
> > > > > > + mutex_lock(_hw->dip_worklist.queuelock);
> > > > > > + dip_work = list_first_entry(_hw->dip_worklist.queue,
> > >
> > > [snip]
> > >
> > > > > > +
> > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > > > + dev_dbg(_dev->pdev->dev,
> > > > > > + "%s: frame_no(%d) has no tuning_data\n",
> > > > > > + __func__, dip_work->frameparams.frame_no);
> > > > > > +
> > > > > > + memcpy(_work->frameparams.tuning_data,
> > > > > > + >tuning_buf, sizeof(buf->tuning_buf));
> > > > >
> > > > > Ditto.
> > > > >
> > > >
> > > > I got it.
> > > >
> > > > > > + memset((char *)buf->tuning_buf.va, 0,
> > > > > > DIP_TUNING_SZ);
> > > > >
> > > > > Ditto.
> > > >
> > > > I got it.
> > > >
> > > > >
> > > > > > + /*
> > > > > > +* When user enqueued without tuning buffer,
> > > > > > +* it would use driver internal buffer.
> > > > > > +* So, tuning_data.va should be 0
> > > > > > +*/
> > > > > > + dip_work->frameparams.tuning_data.va = 0;
> > > > >
> > > > > I don't understand this. We just zeroed the buffer via this kernel VA
> > > > > few
> > > > > lines above, so why would it have to be set to 0?
> > > > >
> > > >
> > > > I will remove this unnecessary line.
> > > >
> > > > > > + }
> > >
> > > After confirming the firmware part, I found that we use this field
> > > (tuning_data.va) to notify firmware if there is no tuning data from
> > > user.
> > >
> > > - frameparams.tuning_data.va is 0: use the default tuning data in
> > >SCP, but we still need to pass
> > >frameparams.tuning_data.pa because
> > >the buffer contains some working
> > >buffer required.
> > > - frameparams.tuning_data.va is not 0: the tuning data was passed from
> > >the user
> > >
> > > Since we should not pass cpu addres to SCP, could I rename tuning_data.va
> > > as tuning_data.cookie, and write a constant value to indicate if SCP
> > > should use its internal default setting or not here?
> > >
> > > For example,
> > > /* SCP uses tuning data passed from userspace*/
> > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA;
> > >
> > > /* SCP uses internal tuning data */
> > > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA;
> >
> > Perhaps we could just call it "present" and set to true or false?
> >
>
> Yes. I would like to use "present" here.
>
> Original:
> struct img_addr {
> u64 va; /* Used by Linux OS access */
> u32 pa; /* Used by CM4 access */
> u32 iova; /* Used by IOMMU HW access */
> } __attribute__ ((__packed__));
>
> struct img_ipi_frameparam {
> u32 index;
> u32 frame_no;
> u64 timestamp;
> u8 type; /* enum mdp_stream_type */
> u8 state;
> u8 num_inputs;
> u8 num_outputs;
> u64 drv_data;
> struct img_input inputs[IMG_MAX_HW_INPUTS];
> struct img_output outputs[IMG_MAX_HW_OUTPUTS];
> struct img_addr tuning_data;
> struct img_addr subfrm_data;
> struct
Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
Hi Tomasz,
On Tue, 2019-06-11 at 17:59 +0900, Tomasz Figa wrote:
> On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen
> wrote:
> >
> > Dear Tomasz,
> >
> > I'd like to elaborate more about the tuning_data.va.
> > Would you like to give us some advice about our improvement proposal inline?
> >
> > Thank you very much.
> >
> >
> > On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote:
> > > Dear Tomasz,
> > >
> > > I appreciate your comment. It is very helpful for us.
> > >
> > >
> > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > new file mode 100644
> > > > > index ..54d2b5f5b802
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > > @@ -0,0 +1,1384 @@
> >
> > [snip]
> >
> > > > > +static void dip_submit_worker(struct work_struct *work)
> > > > > +{
> > > > > + struct mtk_dip_hw_submit_work *dip_submit_work =
> > > > > + container_of(work, struct mtk_dip_hw_submit_work,
> > > > > frame_work);
> > > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw;
> > > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw);
> > > > > + struct mtk_dip_hw_work *dip_work;
> > > > > + struct mtk_dip_hw_subframe *buf;
> > > > > + u32 len, num;
> > > > > + int ret;
> > > > > +
> > > > > + num = atomic_read(_hw->num_composing);
> > > > > +
> > > > > + mutex_lock(_hw->dip_worklist.queuelock);
> > > > > + dip_work = list_first_entry(_hw->dip_worklist.queue,
> >
> > [snip]
> >
> > > > > +
> > > > > + if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > > + dev_dbg(_dev->pdev->dev,
> > > > > + "%s: frame_no(%d) has no tuning_data\n",
> > > > > + __func__, dip_work->frameparams.frame_no);
> > > > > +
> > > > > + memcpy(_work->frameparams.tuning_data,
> > > > > + >tuning_buf, sizeof(buf->tuning_buf));
> > > >
> > > > Ditto.
> > > >
> > >
> > > I got it.
> > >
> > > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> > > >
> > > > Ditto.
> > >
> > > I got it.
> > >
> > > >
> > > > > + /*
> > > > > +* When user enqueued without tuning buffer,
> > > > > +* it would use driver internal buffer.
> > > > > +* So, tuning_data.va should be 0
> > > > > +*/
> > > > > + dip_work->frameparams.tuning_data.va = 0;
> > > >
> > > > I don't understand this. We just zeroed the buffer via this kernel VA
> > > > few
> > > > lines above, so why would it have to be set to 0?
> > > >
> > >
> > > I will remove this unnecessary line.
> > >
> > > > > + }
> >
> > After confirming the firmware part, I found that we use this field
> > (tuning_data.va) to notify firmware if there is no tuning data from
> > user.
> >
> > - frameparams.tuning_data.va is 0: use the default tuning data in
> >SCP, but we still need to pass
> >frameparams.tuning_data.pa because
> >the buffer contains some working
> >buffer required.
> > - frameparams.tuning_data.va is not 0: the tuning data was passed from
> >the user
> >
> > Since we should not pass cpu addres to SCP, could I rename tuning_data.va
> > as tuning_data.cookie, and write a constant value to indicate if SCP
> > should use its internal default setting or not here?
> >
> > For example,
> > /* SCP uses tuning data passed from userspace*/
> > dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA;
> >
> > /* SCP uses internal tuning data */
> > dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA;
>
> Perhaps we could just call it "present" and set to true or false?
>
Yes. I would like to use "present" here.
Original:
struct img_addr {
u64 va; /* Used by Linux OS access */
u32 pa; /* Used by CM4 access */
u32 iova; /* Used by IOMMU HW access */
} __attribute__ ((__packed__));
struct img_ipi_frameparam {
u32 index;
u32 frame_no;
u64 timestamp;
u8 type; /* enum mdp_stream_type */
u8 state;
u8 num_inputs;
u8 num_outputs;
u64 drv_data;
struct img_input inputs[IMG_MAX_HW_INPUTS];
struct img_output outputs[IMG_MAX_HW_OUTPUTS];
struct img_addr tuning_data;
struct img_addr subfrm_data;
struct img_sw_addr config_data;
struct img_sw_addr self_data;
} __attribute__ ((__packed__));
Modified:
struct tuning_buf {
u8 present;
u32 pa; /* Used by CM4 access */
u32 iova; /* Used by IOMMU HW access */
} __attribute__ ((__packed__));
struct img_ipi_frameparam {
/* ... */
struct tuning_buf
Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
On Tue, Jun 11, 2019 at 5:48 PM Frederic Chen
wrote:
>
> Dear Tomasz,
>
> I'd like to elaborate more about the tuning_data.va.
> Would you like to give us some advice about our improvement proposal inline?
>
> Thank you very much.
>
>
> On Wed, 2019-05-22 at 03:14 +0800, Frederic Chen wrote:
> > Dear Tomasz,
> >
> > I appreciate your comment. It is very helpful for us.
> >
> >
> > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > new file mode 100644
> > > > index ..54d2b5f5b802
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > @@ -0,0 +1,1384 @@
>
> [snip]
>
> > > > +static void dip_submit_worker(struct work_struct *work)
> > > > +{
> > > > + struct mtk_dip_hw_submit_work *dip_submit_work =
> > > > + container_of(work, struct mtk_dip_hw_submit_work,
> > > > frame_work);
> > > > + struct mtk_dip_hw *dip_hw = dip_submit_work->dip_hw;
> > > > + struct mtk_dip_dev *dip_dev = mtk_dip_hw_to_dev(dip_hw);
> > > > + struct mtk_dip_hw_work *dip_work;
> > > > + struct mtk_dip_hw_subframe *buf;
> > > > + u32 len, num;
> > > > + int ret;
> > > > +
> > > > + num = atomic_read(_hw->num_composing);
> > > > +
> > > > + mutex_lock(_hw->dip_worklist.queuelock);
> > > > + dip_work = list_first_entry(_hw->dip_worklist.queue,
>
> [snip]
>
> > > > +
> > > > + if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > + dev_dbg(_dev->pdev->dev,
> > > > + "%s: frame_no(%d) has no tuning_data\n",
> > > > + __func__, dip_work->frameparams.frame_no);
> > > > +
> > > > + memcpy(_work->frameparams.tuning_data,
> > > > + >tuning_buf, sizeof(buf->tuning_buf));
> > >
> > > Ditto.
> > >
> >
> > I got it.
> >
> > > > + memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> > >
> > > Ditto.
> >
> > I got it.
> >
> > >
> > > > + /*
> > > > +* When user enqueued without tuning buffer,
> > > > +* it would use driver internal buffer.
> > > > +* So, tuning_data.va should be 0
> > > > +*/
> > > > + dip_work->frameparams.tuning_data.va = 0;
> > >
> > > I don't understand this. We just zeroed the buffer via this kernel VA few
> > > lines above, so why would it have to be set to 0?
> > >
> >
> > I will remove this unnecessary line.
> >
> > > > + }
>
> After confirming the firmware part, I found that we use this field
> (tuning_data.va) to notify firmware if there is no tuning data from
> user.
>
> - frameparams.tuning_data.va is 0: use the default tuning data in
>SCP, but we still need to pass
>frameparams.tuning_data.pa because
>the buffer contains some working
>buffer required.
> - frameparams.tuning_data.va is not 0: the tuning data was passed from
>the user
>
> Since we should not pass cpu addres to SCP, could I rename tuning_data.va
> as tuning_data.cookie, and write a constant value to indicate if SCP
> should use its internal default setting or not here?
>
> For example,
> /* SCP uses tuning data passed from userspace*/
> dip_work->frameparams.tuning_data.cookie = MTK_DIP_USER_TUNING_DATA;
>
> /* SCP uses internal tuning data */
> dip_work->frameparams.tuning_data.cookie = MTK_DIP_DEFAULT_TUNING_DATA;
Perhaps we could just call it "present" and set to true or false?
Best regards,
Tomasz
Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek DIP driver
On Thu, May 23, 2019 at 10:46 PM Frederic Chen
wrote:
>
> Dear Tomasz,
>
> Thank you for your comments.
>
>
> On Wed, 2019-05-22 at 19:25 +0900, Tomasz Figa wrote:
> > Hi Frederic,
> >
> > On Wed, May 22, 2019 at 03:14:15AM +0800, Frederic Chen wrote:
> > > Dear Tomasz,
> > >
> > > I appreciate your comment. It is very helpful for us.
> > >
> >
> > You're welcome. Thanks for replying to all the comments. I'll skip those
> > resolved in my reply to keep the message shorter.
> >
> > >
> > > On Thu, 2019-05-09 at 18:48 +0900, Tomasz Figa wrote:
> > > > Hi Frederic,
> > > >
> > > > On Wed, Apr 17, 2019 at 7:45 PM Frederic Chen
> > > > wrote:
[snip]
> > > > Also a general note - a work can be queued only once. This means that
> > > > current code races when two dip_works are attempted to be queued very
> > > > quickly one after another (or even at the same time from different
> > > > threads).
> > > >
> > > > I can think of two potential options for fixing this:
> > > >
> > > > 1) Loop in the work function until there is nothing to queue to the
> > > > hardware
> > > >anymore - but this needs tricky synchronization, because there is
> > > > still
> > > >short time at the end of the work function when a new dip_work could
> > > > be
> > > >added.
> > > >
> > > > 2) Change this to a kthread that just keeps running in a loop waiting
> > > > for
> > > >some available dip_work to show up and then sending it to the
> > > > firmware.
> > > >This should be simpler, as the kthread shouldn't have a chance to
> > > > miss
> > > >any dip_work queued.
> > > >
> > > > I'm personally in favor of option 2, as it should simplify the
> > > > synchronization.
> > > >
> > >
> > > I would like to re-design this part with a kthread in the next patch.
> >
> > Actually I missed another option. We could have 1 work_struct for 1
> > request and then we could keep using a workqueue. Perhaps that could be
> > simpler than a kthread.
> >
> > Actually, similar approach could be used for the dip_runner_func.
> > Instead of having a kthread looping, we could just have another
> > workqueue and 1 dip_runner_work per 1 request. Then we wouldn't need to
> > do the waiting loop ourselves anymore.
> >
> > Does it make sense?
>
> Yes, it make sense. Let me summarize the modification about the flow.
>
> First, we will have two work_struct in mtk_dip_request.
>
> struct mtk_dip_request {
> struct media_request request;
> //...
> /* Prepare DIP part hardware configurtion */
> struct mtk_dip_hw_submit_work submit_work;
> /* Replace dip_running thread jobs*/
> struct mtk_dip_hw_composing_work composing_work;
> /* Only for composing error handling */
> struct mtk_dip_hw_mdpcb_timeout_work timeout_work;
> };
>
> Second, the overall flow of handling each request is :
>
> 1. mtk_dip_hw_enqueue calls queue_work() to put submit_work into its
>workqueue
> 2. submit_work sends IMG_IPI_FRAME command to SCP to prepare DIP
>hardware configuration
> 3. dip_scp_handler receives the IMG_IPI_FRAME result from SCP
> 4. dip_scp_handler calls queue_work() to put composing_work (instead
>of original dip_running thread jobs) into its workqueue
> 5. composing_work calls dip_mdp_cmdq_send() to finish the mdp part tasks
> 6. dip_mdp_cb_func() trigged by MDP driver calls vb2_buffer_done to
>return the buffer (no workqueue required here)
>
Sounds good to me, but actually then simply making the workqueues
freezable doesn't solve the suspend/resume problem, because the work
functions wouldn't wait for the firmware/hardware completion anymore.
That's also okay, but in this case we need to add some code to suspend
to wait for any pending operations to complete.
Best regards,
Tomasz
