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.
> 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.
> 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.
> -
> - } 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.
> + 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);
Best regards,
Tomasz
