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

[Bingbu Cao <bingbu.cao@linux.intel.com>]

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.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>
>> ---
>> 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);
>