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

[Tomasz Figa <tfiga@chromium.org>]

Hi Bingbu,

On Fri, Feb 15, 2019 at 6:02 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.

Thanks for the patch. Please see my comments inline.

>
> 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 | 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