[PATCH v2] media:staging/intel-ipu3: parameter buffer refactoring

[PATCH v2] media:staging/intel-ipu3: parameter buffer refactoring

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

Current ImgU driver processes and releases the parameter buffer
immediately after queued from user. This does not align with other
image buffers which are grouped in sets and used for the same frame.
If user queues multiple parameter buffers continuously, only the last
one will take effect.
To make consistent buffers usage, this patch changes the parameter
buffer handling and group parameter buffer with other image buffers
for each frame.
Each time driver will queue one more group of buffers when previous
frame processed and buffers consumed by css.

Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com>
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/staging/media/ipu3/ipu3-css.c  |  5 ----
 drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++----------------------
 drivers/staging/media/ipu3/ipu3.c      | 30 ++++++++++++++++++++++
 3 files changed, 46 insertions(+), 35 deletions(-)

---
changes since v1:
- add payload check for parameter buffer
- queue buffer only when previous buffer consumed
---
diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c
index 15ab77e4b766..018f5a266c42 100644
--- a/drivers/staging/media/ipu3/ipu3-css.c
+++ b/drivers/staging/media/ipu3/ipu3-css.c
@@ -2160,11 +2160,6 @@ int imgu_css_set_parameters(struct imgu_css *css, unsigned int pipe,
 	obgrid_size = imgu_css_fw_obgrid_size(bi);
 	stripes = bi->info.isp.sp.iterator.num_stripes ? : 1;
 
-	/*
-	 * TODO(b/118782861): If userspace queues more than 4 buffers, the
-	 * parameters from previous buffers will be overwritten. Fix the driver
-	 * not to allow this.
-	 */
 	imgu_css_pool_get(&css_pipe->pool.parameter_set_info);
 	param_set = imgu_css_pool_last(&css_pipe->pool.parameter_set_info,
 				       0)->vaddr;
diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 9c0352b193a7..87a7919c12f0 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -341,8 +341,10 @@ static void imgu_vb2_buf_queue(struct vb2_buffer *vb)
 	struct imgu_video_device *node =
 		container_of(vb->vb2_queue, struct imgu_video_device, vbq);
 	unsigned int queue = imgu_node_to_queue(node->id);
+	struct imgu_buffer *buf = container_of(vb, struct imgu_buffer,
+					       vid_buf.vbb.vb2_buf);
 	unsigned long need_bytes;
-	unsigned int pipe = node->pipe;
+	unsigned long payload = vb2_get_plane_payload(vb, 0);
 
 	if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE ||
 	    vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT)
@@ -350,42 +352,26 @@ static void imgu_vb2_buf_queue(struct vb2_buffer *vb)
 	else
 		need_bytes = node->vdev_fmt.fmt.pix_mp.plane_fmt[0].sizeimage;
 
-	if (queue == IPU3_CSS_QUEUE_PARAMS) {
-		unsigned long payload = vb2_get_plane_payload(vb, 0);
-		struct vb2_v4l2_buffer *buf =
-			container_of(vb, struct vb2_v4l2_buffer, vb2_buf);
-		int r = -EINVAL;
-
-		if (payload == 0) {
-			payload = need_bytes;
-			vb2_set_plane_payload(vb, 0, payload);
-		}
-		if (payload >= need_bytes)
-			r = imgu_css_set_parameters(&imgu->css, pipe,
-						    vb2_plane_vaddr(vb, 0));
-		buf->flags = V4L2_BUF_FLAG_DONE;
-		vb2_buffer_done(vb, r == 0 ? VB2_BUF_STATE_DONE
-					   : VB2_BUF_STATE_ERROR);
-
-	} else {
-		struct imgu_buffer *buf = container_of(vb, struct imgu_buffer,
-						       vid_buf.vbb.vb2_buf);
+	if (queue == IPU3_CSS_QUEUE_PARAMS && payload && payload < need_bytes) {
+		dev_err(&imgu->pci_dev->dev, "invalid data size for params.");
+		vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
+		return;
+	}
 
-		mutex_lock(&imgu->lock);
+	mutex_lock(&imgu->lock);
+	if (queue != IPU3_CSS_QUEUE_PARAMS)
 		imgu_css_buf_init(&buf->css_buf, queue, buf->map.daddr);
-		list_add_tail(&buf->vid_buf.list,
-			      &node->buffers);
-		mutex_unlock(&imgu->lock);
 
-		vb2_set_plane_payload(&buf->vid_buf.vbb.vb2_buf, 0, need_bytes);
+	list_add_tail(&buf->vid_buf.list, &node->buffers);
+	mutex_unlock(&imgu->lock);
 
-		if (imgu->streaming)
-			imgu_queue_buffers(imgu, false, pipe);
-	}
+	vb2_set_plane_payload(vb, 0, need_bytes);
+
+	if (imgu->streaming)
+		imgu_queue_buffers(imgu, false, node->pipe);
 
 	dev_dbg(&imgu->pci_dev->dev, "%s for pipe %d node %d", __func__,
 		node->pipe, node->id);
-
 }
 
 static int imgu_vb2_queue_setup(struct vb2_queue *vq,
diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c
index d575ac78c8f0..f1045072d535 100644
--- a/drivers/staging/media/ipu3/ipu3.c
+++ b/drivers/staging/media/ipu3/ipu3.c
@@ -236,6 +236,11 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe
 	dev_dbg(&imgu->pci_dev->dev, "Queue buffers to pipe %d", pipe);
 	mutex_lock(&imgu->lock);
 
+	if (!imgu_css_pipe_queue_empty(&imgu->css, pipe)) {
+		mutex_unlock(&imgu->lock);
+		return 0;
+	}
+
 	/* Buffer set is queued to FW only when input buffer is ready */
 	for (node = IMGU_NODE_NUM - 1;
 	     imgu_queue_getbuf(imgu, IMGU_NODE_IN, pipe);
@@ -246,6 +251,31 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe
 			dev_warn(&imgu->pci_dev->dev,
 				 "Vf not enabled, ignore queue");
 			continue;
+		} else if (node == IMGU_NODE_PARAMS &&
+			   imgu_pipe->nodes[node].enabled) {
+			struct vb2_buffer *vb;
+			struct imgu_vb2_buffer *ivb;
+
+			/* No parameters for this frame */
+			if (list_empty(&imgu_pipe->nodes[node].buffers))
+				continue;
+
+			ivb = list_first_entry(&imgu_pipe->nodes[node].buffers,
+					       struct imgu_vb2_buffer, list);
+			vb = &ivb->vbb.vb2_buf;
+			r = imgu_css_set_parameters(&imgu->css, pipe,
+						    vb2_plane_vaddr(vb, 0));
+			if (r) {
+				vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
+				dev_warn(&imgu->pci_dev->dev,
+					 "set parameters failed.");
+				continue;
+			}
+
+			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+			dev_dbg(&imgu->pci_dev->dev,
+				"queue user parameters %d to css.", vb->index);
+			list_del(&ivb->list);
 		} else if (imgu_pipe->queue_enabled[node]) {
 			struct imgu_css_buffer *buf =
 				imgu_queue_getbuf(imgu, node, pipe);
-- 
1.9.1

Re: [PATCH v2] media:staging/intel-ipu3: parameter buffer refactoring

[Tomasz Figa]

Hi Bingbu,

On Fri, Mar 22, 2019 at 8:00 PM <bingbu.cao@intel.com> wrote:
>
> From: Bingbu Cao <bingbu.cao@intel.com>
>
> Current ImgU driver processes and releases the parameter buffer
> immediately after queued from user. This does not align with other
> image buffers which are grouped in sets and used for the same frame.
> If user queues multiple parameter buffers continuously, only the last
> one will take effect.
> To make consistent buffers usage, this patch changes the parameter
> buffer handling and group parameter buffer with other image buffers
> for each frame.
> Each time driver will queue one more group of buffers when previous
> frame processed and buffers consumed by css.
>
> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com>
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  drivers/staging/media/ipu3/ipu3-css.c  |  5 ----
>  drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++----------------------
>  drivers/staging/media/ipu3/ipu3.c      | 30 ++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 35 deletions(-)
>
> ---
> changes since v1:
> - add payload check for parameter buffer
> - queue buffer only when previous buffer consumed
> ---

Thanks for the patch. With the issues pointed by Sakari fixed (+/-
those for follow up patches):

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

By the way, has this been tested on an applicable platform with
applicable platform (e.g. Pixel Slate with Chrome OS) and found no
performance or functional regressions?

Best regards,
Tomasz

Re: [PATCH v2] media:staging/intel-ipu3: parameter buffer refactoring

[Bingbu Cao]

On 4/15/19 5:28 PM, Tomasz Figa wrote:
> Hi Bingbu,
> 
> On Fri, Mar 22, 2019 at 8:00 PM <bingbu.cao@intel.com> wrote:
>>
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> Current ImgU driver processes and releases the parameter buffer
>> immediately after queued from user. This does not align with other
>> image buffers which are grouped in sets and used for the same frame.
>> If user queues multiple parameter buffers continuously, only the last
>> one will take effect.
>> To make consistent buffers usage, this patch changes the parameter
>> buffer handling and group parameter buffer with other image buffers
>> for each frame.
>> Each time driver will queue one more group of buffers when previous
>> frame processed and buffers consumed by css.
>>
>> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>  drivers/staging/media/ipu3/ipu3-css.c  |  5 ----
>>  drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++----------------------
>>  drivers/staging/media/ipu3/ipu3.c      | 30 ++++++++++++++++++++++
>>  3 files changed, 46 insertions(+), 35 deletions(-)
>>
>> ---
>> changes since v1:
>> - add payload check for parameter buffer
>> - queue buffer only when previous buffer consumed
>> ---
> 
> Thanks for the patch. With the issues pointed by Sakari fixed (+/-
> those for follow up patches):
> 
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> 
> By the way, has this been tested on an applicable platform with
> applicable platform (e.g. Pixel Slate with Chrome OS) and found no
> performance or functional regressions?
Hi, Tomasz

Yes, same change was verified in our own integration process with full
test, validation team did not find functional regressions.
However, if you have some extra tests which can check the performance, I
will add them into the test cases list.

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2] media:staging/intel-ipu3: parameter buffer refactoring

[Tomasz Figa]

Hi Bingbu,

On Mon, Apr 15, 2019 at 6:42 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
>
>
> On 4/15/19 5:28 PM, Tomasz Figa wrote:
> > Hi Bingbu,
> >
> > On Fri, Mar 22, 2019 at 8:00 PM <bingbu.cao@intel.com> wrote:
> >>
> >> From: Bingbu Cao <bingbu.cao@intel.com>
> >>
> >> Current ImgU driver processes and releases the parameter buffer
> >> immediately after queued from user. This does not align with other
> >> image buffers which are grouped in sets and used for the same frame.
> >> If user queues multiple parameter buffers continuously, only the last
> >> one will take effect.
> >> To make consistent buffers usage, this patch changes the parameter
> >> buffer handling and group parameter buffer with other image buffers
> >> for each frame.
> >> Each time driver will queue one more group of buffers when previous
> >> frame processed and buffers consumed by css.
> >>
> >> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> >> ---
> >>  drivers/staging/media/ipu3/ipu3-css.c  |  5 ----
> >>  drivers/staging/media/ipu3/ipu3-v4l2.c | 46 ++++++++++++----------------------
> >>  drivers/staging/media/ipu3/ipu3.c      | 30 ++++++++++++++++++++++
> >>  3 files changed, 46 insertions(+), 35 deletions(-)
> >>
> >> ---
> >> changes since v1:
> >> - add payload check for parameter buffer
> >> - queue buffer only when previous buffer consumed
> >> ---
> >
> > Thanks for the patch. With the issues pointed by Sakari fixed (+/-
> > those for follow up patches):
> >
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> >
> > By the way, has this been tested on an applicable platform with
> > applicable platform (e.g. Pixel Slate with Chrome OS) and found no
> > performance or functional regressions?
> Hi, Tomasz
>
> Yes, same change was verified in our own integration process with full
> test, validation team did not find functional regressions.
> However, if you have some extra tests which can check the performance, I
> will add them into the test cases list.

For example, the camera_HAL3Perf test on Chrome OS could be used to
measure various latencies, with a side note that some of them depend
on environment lighting, so should be run in good lighting conditions.

Best regards,
Tomasz