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(&pdev->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(&pdev->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 = &dip_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(&dip_hw->num_composing); > > > > > > > + > > > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > > > > > [snip] > > > > > > > > > > > + > > > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > > > + > > > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > > > + &buf->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 fr
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(&dip_hw->num_composing); > > > > > > + > > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > > > [snip] > > > > > > > > > + > > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > > + > > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > > + &buf->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
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(&dip_hw->num_composing); > > > > > + > > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > > > [snip] > > > > > > > + > > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > > + __func__, dip_work->frameparams.frame_no); > > > > > + > > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > > + &buf->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 { /* ... */ str
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(&dip_hw->num_composing); > > > > + > > > > + mutex_lock(&dip_hw->dip_worklist.queuelock); > > > > + dip_work = list_first_entry(&dip_hw->dip_worklist.queue, > > [snip] > > > > > + > > > > + if (dip_work->frameparams.tuning_data.pa == 0) { > > > > + dev_dbg(&dip_dev->pdev->dev, > > > > + "%s: frame_no(%d) has no tuning_data\n", > > > > + __func__, dip_work->frameparams.frame_no); > > > > + > > > > + memcpy(&dip_work->frameparams.tuning_data, > > > > + &buf->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