On 03/12/2019 01:33 PM, Tomasz Figa wrote:
Hi Bingbu,
On Fri, Feb 15, 2019 at 6:02 PM <bingbu.cao@xxxxxxxxx> wrote:
From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
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.
Thanks for the patch. Please see my comments inline.
Signed-off-by: Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
---
drivers/staging/media/ipu3/ipu3-css.c | 5 -----
drivers/staging/media/ipu3/ipu3-v4l2.c | 41 ++++++++--------------------------
drivers/staging/media/ipu3/ipu3.c | 24 ++++++++++++++++++++
3 files changed, 33 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c
index b9354d2bb692..bcb1d436bc98 100644
--- a/drivers/staging/media/ipu3/ipu3-css.c
+++ b/drivers/staging/media/ipu3/ipu3-css.c
@@ -2160,11 +2160,6 @@ int ipu3_css_set_parameters(struct ipu3_css *css, unsigned int pipe,
obgrid_size = ipu3_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.
- */
Wouldn't this still happen even with current patch?
imgu_queue_buffers() supposedly queues "as many buffers to CSS as
possible". This means that if the userspace queues more than 4
complete frames, we still end up overwriting the parameter buffers in
the pool. Please correct me if I'm wrong.
The parameter buffers are queued to CSS sequentially and queue one
parameter along with one input buffer once ready, all the data and
parameter buffers are tied together to queue to the CSS. If userspace
queue more parameter buffers then input buffer, they are pending on the
buffer list.
The parameters pool actually was just used to keep the last valid
parameter, if necessary it will be used to copy and combine with latest
parameter to queue. When driver reach at ipu3_css_set_parameters(), driver
will queue the parameter soon, no buffers pending as one patch before
change the pool mechanism.
ipu3_css_pool_get(&css_pipe->pool.parameter_set_info);
param_set = ipu3_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 e758a650ad2b..7812c7324893 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -341,8 +341,9 @@ static void ipu3_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;
if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE ||
vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT)
@@ -350,42 +351,18 @@ static void ipu3_vb2_buf_queue(struct vb2_buffer *vb)
Looks like this might need a rebase. This function seems to be called
imgu_vb2_buf_queue() in current linux-next. Other functions seem to
have been changed from ipu3_ to imgu_ as well.
Thanks, I will rebase it.
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 = ipu3_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);
This patch removes the check of the buffer contents. We should
probably check the payload size in the vb2 .buf_prepare callback and
fail if it doesn't match the parameter struct size.
Thanks, will add the check in next patch.
-
- } else {
- struct imgu_buffer *buf = container_of(vb, struct imgu_buffer,
- vid_buf.vbb.vb2_buf);
-
- mutex_lock(&imgu->lock);
+ mutex_lock(&imgu->lock);
+ if (queue != IPU3_CSS_QUEUE_PARAMS)
ipu3_css_buf_init(&buf->css_buf, queue, buf->map.daddr);
- list_add_tail(&buf->vid_buf.list,
- &node->buffers);
- mutex_unlock(&imgu->lock);
+ 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);
-
- if (imgu->streaming)
- imgu_queue_buffers(imgu, false, pipe);
- }
+ vb2_set_plane_payload(vb, 0, need_bytes);
Uhm, the driver is expected to only set the payload for CAPTURE buffers.
Ack.
+ 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 ipu3_vb2_queue_setup(struct vb2_queue *vq,
diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c
index 839d9398f8e9..25e121eebee2 100644
--- a/drivers/staging/media/ipu3/ipu3.c
+++ b/drivers/staging/media/ipu3/ipu3.c
@@ -246,6 +246,30 @@ 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 ipu3_vb2_buffer *ivb;
+
+ if (list_empty(&imgu_pipe->nodes[node].buffers))
+ /* No parameters for this frame. */
+ continue;
+ ivb = list_first_entry(&imgu_pipe->nodes[node].buffers,
+ struct ipu3_vb2_buffer, list);
+ vb = &ivb->vbb.vb2_buf;
+ r = ipu3_css_set_parameters(&imgu->css, pipe,
+ vb2_plane_vaddr(vb, 0));
+ if (r) {
+ vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
+ dev_err(&imgu->pci_dev->dev,
+ "set parameters failed.\n");
+ } else {
+ vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+ dev_dbg(&imgu->pci_dev->dev,
+ "queue user parameters %d to css.\n",
+ vb->index);
+ }
+ list_del(&ivb->list);
nit: I'd rewrite the code as below, to maintain the kernel convention
of using ifs to handle special cases and keep the regular code flow at
the same level of indentation.
r = ipu3_css_set_parameters(&imgu->css, pipe,
vb2_plane_vaddr(vb, 0));
list_del(&ivb->list);
if (r) {
vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
dev_err(&imgu->pci_dev->dev,
"set parameters failed.\n");
continue;
}
vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
dev_dbg(&imgu->pci_dev->dev,
"queue user parameters %d to css.\n",
vb->index);
Ack.
Best regards,
Tomasz
