On 4/8/19 8:26 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> Thanks for the patch.
>
> On Fri, Mar 22, 2019 at 07:14:45PM +0800, bingbu....@intel.com wrote:
>> From: Bingbu Cao <bingbu....@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....@intel.com>
>> Signed-off-by: Bingbu Cao <bingbu....@intel.com>
>> ---
>> changes since v1:
>> - add payload check for parameter buffer
>> - queue buffer only when previous buffer consumed
>> ---
>> 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(-)
>>
>> 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.cguess it'd take
significant changes to the code handling them, and there are more important
things to do. Just a note.
>> +++ 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) {guess it'd take
significant changes to the code handling them, and there are more important
things to do. Just a note.
>> - 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);
>
> This would better be done in buf_prepare callback --- you can return an
> error right away then. But it could well be another patch.
OK, thanks, I will add this into my TODO list.
>
>> + 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) {
>
> Ideally the parameters should be validated earlier but I guess it'd take
> significant changes to the code handling them, and there are more important
> things to do. Just a note.
>
>> + 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);
>
> %d -> %u.
>
> I can fix that while applying the patch.
Thanks.
>
>> + list_del(&ivb->list);
>> } else if (imgu_pipe->queue_enabled[node]) {
>> struct imgu_css_buffer *buf =
>> imgu_queue_getbuf(imgu, node, pipe);
>
