subject:"\[RFC PATCH 5\/9\] media\: vb2\: add support for requests"

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-17 Thread Hans Verkuil

On 01/17/18 09:01, Alexandre Courbot wrote:
> On Tue, Jan 16, 2018 at 7:37 PM, Hans Verkuil  wrote:
>> On 01/16/2018 10:39 AM, Alexandre Courbot wrote:
>>> On Mon, Jan 15, 2018 at 6:07 PM, Hans Verkuil  wrote:
 On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Add throttling support for buffers when requests are in use on a given
>>> queue. Buffers associated to a request are kept into the vb2 queue until
>>> the request becomes active, at which point all the buffers are passed to
>>> the driver. The queue can also signal that is has processed all of a
>>> request's buffers.
>>>
>>> Also add support for the request parameter when handling the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>>> 
>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>>  include/media/videobuf2-core.h   | 25 +-
>>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index cb115ba6a1d2..c01038b7962a 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   state != VB2_BUF_STATE_REQUEUEING))
>>>   state = VB2_BUF_STATE_ERROR;
>>>
>>> + WARN_ON(vb->request != q->cur_req);
>>
>> What's the reason for this WARN_ON? It's not immediately obvious to me.
>
> This is a safeguard against driver bugs: a buffer should not complete
> unless it is part of the request being currently processed.
>
>>
>>> +
>>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>>   /*
>>>* Although this is not a callback, it still does have to balance
>>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   /* Add the buffer to the done buffers list */
>>>   list_add_tail(>done_entry, >done_list);
>>>   vb->state = state;
>>> +
>>> + if (q->cur_req) {
>>> + WARN_ON(q->req_buf_cnt < 1);
>>> +
>>> + if (--q->req_buf_cnt == 0)
>>> + q->cur_req = NULL;
>>> + }
>>>   }
>>>   atomic_dec(>owned_by_drv_count);
>>>   spin_unlock_irqrestore(>done_lock, flags);
>>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>>> unsigned int index, void *pb)
>>>  }
>>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>>
>>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>>> +{
>>> + struct vb2_buffer *vb;
>>> +
>>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>>> + if (vb->request == q->cur_req)
>>> + __enqueue_in_driver(vb);
>>> + }
>>> +}
>>
>> I think this will clash big time with the v4l2 fence patch series...
>
> Indeed, but on the other hand I was not a big fan of going through the
> whole list. :) So I welcome the extra throttling introduced by the
> fence series.

 There is only throttling if fences are used by userspace. Otherwise there
 is no change.

>
>>
>>> +
>>>  /**
>>>   * vb2_start_streaming() - Attempt to start streaming.
>>>   * @q:   videobuf2 queue
>>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue 
>>> *q)
>>>* If any buffers were queued before streamon,
>>>* we can now pass them to driver for processing.
>>>*/
>>> - list_for_each_entry(vb, >queued_list, queued_entry)
>>> - __enqueue_in_driver(vb);
>>> + vb2_queue_enqueue_current_buffers(q);
>>>
>>>   /* Tell the driver to start streaming */
>>>   q->start_streaming_called = 1;
>>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue 
>>> *q)
>>>   return ret;
>>>  }
>>>
>>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>>> +   struct media_request *req, void *pb)
>>>  {
>>>   struct vb2_buffer *vb;
>>>   int ret;
>>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned 
>>> int index, void *pb)
>>>   q->queued_count++;
>>>

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-17 Thread Alexandre Courbot

On Tue, Jan 16, 2018 at 7:37 PM, Hans Verkuil  wrote:
> On 01/16/2018 10:39 AM, Alexandre Courbot wrote:
>> On Mon, Jan 15, 2018 at 6:07 PM, Hans Verkuil  wrote:
>>> On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
 On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> Add throttling support for buffers when requests are in use on a given
>> queue. Buffers associated to a request are kept into the vb2 queue until
>> the request becomes active, at which point all the buffers are passed to
>> the driver. The queue can also signal that is has processed all of a
>> request's buffers.
>>
>> Also add support for the request parameter when handling the QBUF ioctl.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>> 
>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>  include/media/videobuf2-core.h   | 25 +-
>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index cb115ba6a1d2..c01038b7962a 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   state != VB2_BUF_STATE_REQUEUEING))
>>   state = VB2_BUF_STATE_ERROR;
>>
>> + WARN_ON(vb->request != q->cur_req);
>
> What's the reason for this WARN_ON? It's not immediately obvious to me.

 This is a safeguard against driver bugs: a buffer should not complete
 unless it is part of the request being currently processed.

>
>> +
>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>   /*
>>* Although this is not a callback, it still does have to balance
>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   /* Add the buffer to the done buffers list */
>>   list_add_tail(>done_entry, >done_list);
>>   vb->state = state;
>> +
>> + if (q->cur_req) {
>> + WARN_ON(q->req_buf_cnt < 1);
>> +
>> + if (--q->req_buf_cnt == 0)
>> + q->cur_req = NULL;
>> + }
>>   }
>>   atomic_dec(>owned_by_drv_count);
>>   spin_unlock_irqrestore(>done_lock, flags);
>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>> unsigned int index, void *pb)
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>> +{
>> + struct vb2_buffer *vb;
>> +
>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>> + if (vb->request == q->cur_req)
>> + __enqueue_in_driver(vb);
>> + }
>> +}
>
> I think this will clash big time with the v4l2 fence patch series...

 Indeed, but on the other hand I was not a big fan of going through the
 whole list. :) So I welcome the extra throttling introduced by the
 fence series.
>>>
>>> There is only throttling if fences are used by userspace. Otherwise there
>>> is no change.
>>>

>
>> +
>>  /**
>>   * vb2_start_streaming() - Attempt to start streaming.
>>   * @q:   videobuf2 queue
>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>* If any buffers were queued before streamon,
>>* we can now pass them to driver for processing.
>>*/
>> - list_for_each_entry(vb, >queued_list, queued_entry)
>> - __enqueue_in_driver(vb);
>> + vb2_queue_enqueue_current_buffers(q);
>>
>>   /* Tell the driver to start streaming */
>>   q->start_streaming_called = 1;
>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>   return ret;
>>  }
>>
>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>> +   struct media_request *req, void *pb)
>>  {
>>   struct vb2_buffer *vb;
>>   int ret;
>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned 
>> int index, void *pb)
>>   q->queued_count++;
>>   q->waiting_for_buffers = false;
>>   vb->state = VB2_BUF_STATE_QUEUED;
>> + vb->request = req;
>>
>>   if (pb)
>>   call_void_bufop(q, copy_timestamp,

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-16 Thread Hans Verkuil

On 01/16/2018 10:39 AM, Alexandre Courbot wrote:
> On Mon, Jan 15, 2018 at 6:07 PM, Hans Verkuil  wrote:
>> On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
>>> On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
 On 12/15/17 08:56, Alexandre Courbot wrote:
> Add throttling support for buffers when requests are in use on a given
> queue. Buffers associated to a request are kept into the vb2 queue until
> the request becomes active, at which point all the buffers are passed to
> the driver. The queue can also signal that is has processed all of a
> request's buffers.
>
> Also add support for the request parameter when handling the QBUF ioctl.
>
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 59 
> 
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>  include/media/videobuf2-core.h   | 25 +-
>  3 files changed, 104 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index cb115ba6a1d2..c01038b7962a 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
> vb2_buffer_state state)
>   state != VB2_BUF_STATE_REQUEUEING))
>   state = VB2_BUF_STATE_ERROR;
>
> + WARN_ON(vb->request != q->cur_req);

 What's the reason for this WARN_ON? It's not immediately obvious to me.
>>>
>>> This is a safeguard against driver bugs: a buffer should not complete
>>> unless it is part of the request being currently processed.
>>>

> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>   /*
>* Although this is not a callback, it still does have to balance
> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
> vb2_buffer_state state)
>   /* Add the buffer to the done buffers list */
>   list_add_tail(>done_entry, >done_list);
>   vb->state = state;
> +
> + if (q->cur_req) {
> + WARN_ON(q->req_buf_cnt < 1);
> +
> + if (--q->req_buf_cnt == 0)
> + q->cur_req = NULL;
> + }
>   }
>   atomic_dec(>owned_by_drv_count);
>   spin_unlock_irqrestore(>done_lock, flags);
> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
> unsigned int index, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>
> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
> +{
> + struct vb2_buffer *vb;
> +
> + list_for_each_entry(vb, >queued_list, queued_entry) {
> + if (vb->request == q->cur_req)
> + __enqueue_in_driver(vb);
> + }
> +}

 I think this will clash big time with the v4l2 fence patch series...
>>>
>>> Indeed, but on the other hand I was not a big fan of going through the
>>> whole list. :) So I welcome the extra throttling introduced by the
>>> fence series.
>>
>> There is only throttling if fences are used by userspace. Otherwise there
>> is no change.
>>
>>>

> +
>  /**
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:   videobuf2 queue
> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>* If any buffers were queued before streamon,
>* we can now pass them to driver for processing.
>*/
> - list_for_each_entry(vb, >queued_list, queued_entry)
> - __enqueue_in_driver(vb);
> + vb2_queue_enqueue_current_buffers(q);
>
>   /* Tell the driver to start streaming */
>   q->start_streaming_called = 1;
> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>   return ret;
>  }
>
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
> +   struct media_request *req, void *pb)
>  {
>   struct vb2_buffer *vb;
>   int ret;
> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb)
>   q->queued_count++;
>   q->waiting_for_buffers = false;
>   vb->state = VB2_BUF_STATE_QUEUED;
> + vb->request = req;
>
>   if (pb)
>   call_void_bufop(q, copy_timestamp, vb, pb);
> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned 
> int index, void *pb)
>   /*
>* If already streaming, give the buffer to driver

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-16 Thread Alexandre Courbot

On Mon, Jan 15, 2018 at 6:07 PM, Hans Verkuil  wrote:
> On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
>> On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
>>> On 12/15/17 08:56, Alexandre Courbot wrote:
 Add throttling support for buffers when requests are in use on a given
 queue. Buffers associated to a request are kept into the vb2 queue until
 the request becomes active, at which point all the buffers are passed to
 the driver. The queue can also signal that is has processed all of a
 request's buffers.

 Also add support for the request parameter when handling the QBUF ioctl.

 Signed-off-by: Alexandre Courbot 
 ---
  drivers/media/v4l2-core/videobuf2-core.c | 59 
 
  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
  include/media/videobuf2-core.h   | 25 +-
  3 files changed, 104 insertions(+), 9 deletions(-)

 diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
 b/drivers/media/v4l2-core/videobuf2-core.c
 index cb115ba6a1d2..c01038b7962a 100644
 --- a/drivers/media/v4l2-core/videobuf2-core.c
 +++ b/drivers/media/v4l2-core/videobuf2-core.c
 @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
 vb2_buffer_state state)
   state != VB2_BUF_STATE_REQUEUEING))
   state = VB2_BUF_STATE_ERROR;

 + WARN_ON(vb->request != q->cur_req);
>>>
>>> What's the reason for this WARN_ON? It's not immediately obvious to me.
>>
>> This is a safeguard against driver bugs: a buffer should not complete
>> unless it is part of the request being currently processed.
>>
>>>
 +
  #ifdef CONFIG_VIDEO_ADV_DEBUG
   /*
* Although this is not a callback, it still does have to balance
 @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
 vb2_buffer_state state)
   /* Add the buffer to the done buffers list */
   list_add_tail(>done_entry, >done_list);
   vb->state = state;
 +
 + if (q->cur_req) {
 + WARN_ON(q->req_buf_cnt < 1);
 +
 + if (--q->req_buf_cnt == 0)
 + q->cur_req = NULL;
 + }
   }
   atomic_dec(>owned_by_drv_count);
   spin_unlock_irqrestore(>done_lock, flags);
 @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
 unsigned int index, void *pb)
  }
  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);

 +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
 +{
 + struct vb2_buffer *vb;
 +
 + list_for_each_entry(vb, >queued_list, queued_entry) {
 + if (vb->request == q->cur_req)
 + __enqueue_in_driver(vb);
 + }
 +}
>>>
>>> I think this will clash big time with the v4l2 fence patch series...
>>
>> Indeed, but on the other hand I was not a big fan of going through the
>> whole list. :) So I welcome the extra throttling introduced by the
>> fence series.
>
> There is only throttling if fences are used by userspace. Otherwise there
> is no change.
>
>>
>>>
 +
  /**
   * vb2_start_streaming() - Attempt to start streaming.
   * @q:   videobuf2 queue
 @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
* If any buffers were queued before streamon,
* we can now pass them to driver for processing.
*/
 - list_for_each_entry(vb, >queued_list, queued_entry)
 - __enqueue_in_driver(vb);
 + vb2_queue_enqueue_current_buffers(q);

   /* Tell the driver to start streaming */
   q->start_streaming_called = 1;
 @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
   return ret;
  }

 -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
 +   struct media_request *req, void *pb)
  {
   struct vb2_buffer *vb;
   int ret;
 @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
 index, void *pb)
   q->queued_count++;
   q->waiting_for_buffers = false;
   vb->state = VB2_BUF_STATE_QUEUED;
 + vb->request = req;

   if (pb)
   call_void_bufop(q, copy_timestamp, vb, pb);
 @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
 index, void *pb)
   /*
* If already streaming, give the buffer to driver for processing.
* If not, the buffer will be given to driver on next streamon.
 +  *
 +  * If using the request API, the buffer will be given to

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-15 Thread Hans Verkuil

On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Add throttling support for buffers when requests are in use on a given
>>> queue. Buffers associated to a request are kept into the vb2 queue until
>>> the request becomes active, at which point all the buffers are passed to
>>> the driver. The queue can also signal that is has processed all of a
>>> request's buffers.
>>>
>>> Also add support for the request parameter when handling the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>>> 
>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>>  include/media/videobuf2-core.h   | 25 +-
>>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index cb115ba6a1d2..c01038b7962a 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   state != VB2_BUF_STATE_REQUEUEING))
>>>   state = VB2_BUF_STATE_ERROR;
>>>
>>> + WARN_ON(vb->request != q->cur_req);
>>
>> What's the reason for this WARN_ON? It's not immediately obvious to me.
> 
> This is a safeguard against driver bugs: a buffer should not complete
> unless it is part of the request being currently processed.
> 
>>
>>> +
>>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>>   /*
>>>* Although this is not a callback, it still does have to balance
>>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>>> vb2_buffer_state state)
>>>   /* Add the buffer to the done buffers list */
>>>   list_add_tail(>done_entry, >done_list);
>>>   vb->state = state;
>>> +
>>> + if (q->cur_req) {
>>> + WARN_ON(q->req_buf_cnt < 1);
>>> +
>>> + if (--q->req_buf_cnt == 0)
>>> + q->cur_req = NULL;
>>> + }
>>>   }
>>>   atomic_dec(>owned_by_drv_count);
>>>   spin_unlock_irqrestore(>done_lock, flags);
>>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>>> unsigned int index, void *pb)
>>>  }
>>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>>
>>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>>> +{
>>> + struct vb2_buffer *vb;
>>> +
>>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>>> + if (vb->request == q->cur_req)
>>> + __enqueue_in_driver(vb);
>>> + }
>>> +}
>>
>> I think this will clash big time with the v4l2 fence patch series...
> 
> Indeed, but on the other hand I was not a big fan of going through the
> whole list. :) So I welcome the extra throttling introduced by the
> fence series.

There is only throttling if fences are used by userspace. Otherwise there
is no change.

> 
>>
>>> +
>>>  /**
>>>   * vb2_start_streaming() - Attempt to start streaming.
>>>   * @q:   videobuf2 queue
>>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>>* If any buffers were queued before streamon,
>>>* we can now pass them to driver for processing.
>>>*/
>>> - list_for_each_entry(vb, >queued_list, queued_entry)
>>> - __enqueue_in_driver(vb);
>>> + vb2_queue_enqueue_current_buffers(q);
>>>
>>>   /* Tell the driver to start streaming */
>>>   q->start_streaming_called = 1;
>>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>>   return ret;
>>>  }
>>>
>>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>>> +   struct media_request *req, void *pb)
>>>  {
>>>   struct vb2_buffer *vb;
>>>   int ret;
>>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>>> index, void *pb)
>>>   q->queued_count++;
>>>   q->waiting_for_buffers = false;
>>>   vb->state = VB2_BUF_STATE_QUEUED;
>>> + vb->request = req;
>>>
>>>   if (pb)
>>>   call_void_bufop(q, copy_timestamp, vb, pb);
>>> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>>> index, void *pb)
>>>   /*
>>>* If already streaming, give the buffer to driver for processing.
>>>* If not, the buffer will be given to driver on next streamon.
>>> +  *
>>> +  * If using the request API, the buffer will be given to the driver
>>> +  * when the request becomes active.
>>>*/
>>> - if (q->start_streaming_called)
>>> + if (q->start_streaming_called && !req)
>>>

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-15 Thread Alexandre Courbot

On Fri, Jan 12, 2018 at 7:49 PM, Hans Verkuil  wrote:
> On 12/15/17 08:56, Alexandre Courbot wrote:
>> Add throttling support for buffers when requests are in use on a given
>> queue. Buffers associated to a request are kept into the vb2 queue until
>> the request becomes active, at which point all the buffers are passed to
>> the driver. The queue can also signal that is has processed all of a
>> request's buffers.
>>
>> Also add support for the request parameter when handling the QBUF ioctl.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 59 
>> 
>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>>  include/media/videobuf2-core.h   | 25 +-
>>  3 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index cb115ba6a1d2..c01038b7962a 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   state != VB2_BUF_STATE_REQUEUEING))
>>   state = VB2_BUF_STATE_ERROR;
>>
>> + WARN_ON(vb->request != q->cur_req);
>
> What's the reason for this WARN_ON? It's not immediately obvious to me.

This is a safeguard against driver bugs: a buffer should not complete
unless it is part of the request being currently processed.

>
>> +
>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>>   /*
>>* Although this is not a callback, it still does have to balance
>> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
>> vb2_buffer_state state)
>>   /* Add the buffer to the done buffers list */
>>   list_add_tail(>done_entry, >done_list);
>>   vb->state = state;
>> +
>> + if (q->cur_req) {
>> + WARN_ON(q->req_buf_cnt < 1);
>> +
>> + if (--q->req_buf_cnt == 0)
>> + q->cur_req = NULL;
>> + }
>>   }
>>   atomic_dec(>owned_by_drv_count);
>>   spin_unlock_irqrestore(>done_lock, flags);
>> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>> unsigned int index, void *pb)
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
>> +{
>> + struct vb2_buffer *vb;
>> +
>> + list_for_each_entry(vb, >queued_list, queued_entry) {
>> + if (vb->request == q->cur_req)
>> + __enqueue_in_driver(vb);
>> + }
>> +}
>
> I think this will clash big time with the v4l2 fence patch series...

Indeed, but on the other hand I was not a big fan of going through the
whole list. :) So I welcome the extra throttling introduced by the
fence series.

>
>> +
>>  /**
>>   * vb2_start_streaming() - Attempt to start streaming.
>>   * @q:   videobuf2 queue
>> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>* If any buffers were queued before streamon,
>>* we can now pass them to driver for processing.
>>*/
>> - list_for_each_entry(vb, >queued_list, queued_entry)
>> - __enqueue_in_driver(vb);
>> + vb2_queue_enqueue_current_buffers(q);
>>
>>   /* Tell the driver to start streaming */
>>   q->start_streaming_called = 1;
>> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>   return ret;
>>  }
>>
>> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
>> +   struct media_request *req, void *pb)
>>  {
>>   struct vb2_buffer *vb;
>>   int ret;
>> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>   q->queued_count++;
>>   q->waiting_for_buffers = false;
>>   vb->state = VB2_BUF_STATE_QUEUED;
>> + vb->request = req;
>>
>>   if (pb)
>>   call_void_bufop(q, copy_timestamp, vb, pb);
>> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>   /*
>>* If already streaming, give the buffer to driver for processing.
>>* If not, the buffer will be given to driver on next streamon.
>> +  *
>> +  * If using the request API, the buffer will be given to the driver
>> +  * when the request becomes active.
>>*/
>> - if (q->start_streaming_called)
>> + if (q->start_streaming_called && !req)
>>   __enqueue_in_driver(vb);
>>
>>   /* Fill buffer information for the userspace */
>> @@ -1427,6 +1450,28 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>> index, void *pb)
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>>
>> +void vb2_queue_start_request(struct vb2_queue *q,

Re: [RFC PATCH 5/9] media: vb2: add support for requests

2018-01-12 Thread Hans Verkuil

On 12/15/17 08:56, Alexandre Courbot wrote:
> Add throttling support for buffers when requests are in use on a given
> queue. Buffers associated to a request are kept into the vb2 queue until
> the request becomes active, at which point all the buffers are passed to
> the driver. The queue can also signal that is has processed all of a
> request's buffers.
> 
> Also add support for the request parameter when handling the QBUF ioctl.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 59 
> 
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
>  include/media/videobuf2-core.h   | 25 +-
>  3 files changed, 104 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index cb115ba6a1d2..c01038b7962a 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
> vb2_buffer_state state)
>   state != VB2_BUF_STATE_REQUEUEING))
>   state = VB2_BUF_STATE_ERROR;
>  
> + WARN_ON(vb->request != q->cur_req);

What's the reason for this WARN_ON? It's not immediately obvious to me.

> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>   /*
>* Although this is not a callback, it still does have to balance
> @@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
> vb2_buffer_state state)
>   /* Add the buffer to the done buffers list */
>   list_add_tail(>done_entry, >done_list);
>   vb->state = state;
> +
> + if (q->cur_req) {
> + WARN_ON(q->req_buf_cnt < 1);
> +
> + if (--q->req_buf_cnt == 0)
> + q->cur_req = NULL;
> + }
>   }
>   atomic_dec(>owned_by_drv_count);
>   spin_unlock_irqrestore(>done_lock, flags);
> @@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned 
> int index, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>  
> +static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
> +{
> + struct vb2_buffer *vb;
> +
> + list_for_each_entry(vb, >queued_list, queued_entry) {
> + if (vb->request == q->cur_req)
> + __enqueue_in_driver(vb);
> + }
> +}

I think this will clash big time with the v4l2 fence patch series...

> +
>  /**
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:   videobuf2 queue
> @@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>* If any buffers were queued before streamon,
>* we can now pass them to driver for processing.
>*/
> - list_for_each_entry(vb, >queued_list, queued_entry)
> - __enqueue_in_driver(vb);
> + vb2_queue_enqueue_current_buffers(q);
>  
>   /* Tell the driver to start streaming */
>   q->start_streaming_called = 1;
> @@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>   return ret;
>  }
>  
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
> +   struct media_request *req, void *pb)
>  {
>   struct vb2_buffer *vb;
>   int ret;
> @@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb)
>   q->queued_count++;
>   q->waiting_for_buffers = false;
>   vb->state = VB2_BUF_STATE_QUEUED;
> + vb->request = req;
>  
>   if (pb)
>   call_void_bufop(q, copy_timestamp, vb, pb);
> @@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb)
>   /*
>* If already streaming, give the buffer to driver for processing.
>* If not, the buffer will be given to driver on next streamon.
> +  *
> +  * If using the request API, the buffer will be given to the driver
> +  * when the request becomes active.
>*/
> - if (q->start_streaming_called)
> + if (q->start_streaming_called && !req)
>   __enqueue_in_driver(vb);
>  
>   /* Fill buffer information for the userspace */
> @@ -1427,6 +1450,28 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  
> +void vb2_queue_start_request(struct vb2_queue *q, struct media_request *req)
> +{
> + struct vb2_buffer *vb;
> +
> + q->req_buf_cnt = 0;
> + list_for_each_entry(vb, >queued_list, queued_entry) {
> + if (vb->request == req)
> + ++q->req_buf_cnt;
> + }
> +
> + /* only consider the request if we actually have buffers for it */
> + if (q->req_buf_cnt == 0)
> + return;
> +
> + q->cur_req = req;
> +
> + /* If not streaming yet, we will enqueue

[RFC PATCH 5/9] media: vb2: add support for requests

2017-12-14 Thread Alexandre Courbot

Add throttling support for buffers when requests are in use on a given
queue. Buffers associated to a request are kept into the vb2 queue until
the request becomes active, at which point all the buffers are passed to
the driver. The queue can also signal that is has processed all of a
request's buffers.

Also add support for the request parameter when handling the QBUF ioctl.

Signed-off-by: Alexandre Courbot 
---
 drivers/media/v4l2-core/videobuf2-core.c | 59 
 drivers/media/v4l2-core/videobuf2-v4l2.c | 29 +++-
 include/media/videobuf2-core.h   | 25 +-
 3 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index cb115ba6a1d2..c01038b7962a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -898,6 +898,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
vb2_buffer_state state)
state != VB2_BUF_STATE_REQUEUEING))
state = VB2_BUF_STATE_ERROR;
 
+   WARN_ON(vb->request != q->cur_req);
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
/*
 * Although this is not a callback, it still does have to balance
@@ -920,6 +922,13 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
vb2_buffer_state state)
/* Add the buffer to the done buffers list */
list_add_tail(>done_entry, >done_list);
vb->state = state;
+
+   if (q->cur_req) {
+   WARN_ON(q->req_buf_cnt < 1);
+
+   if (--q->req_buf_cnt == 0)
+   q->cur_req = NULL;
+   }
}
atomic_dec(>owned_by_drv_count);
spin_unlock_irqrestore(>done_lock, flags);
@@ -1298,6 +1307,16 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned 
int index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
 
+static void vb2_queue_enqueue_current_buffers(struct vb2_queue *q)
+{
+   struct vb2_buffer *vb;
+
+   list_for_each_entry(vb, >queued_list, queued_entry) {
+   if (vb->request == q->cur_req)
+   __enqueue_in_driver(vb);
+   }
+}
+
 /**
  * vb2_start_streaming() - Attempt to start streaming.
  * @q: videobuf2 queue
@@ -1318,8 +1337,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
 * If any buffers were queued before streamon,
 * we can now pass them to driver for processing.
 */
-   list_for_each_entry(vb, >queued_list, queued_entry)
-   __enqueue_in_driver(vb);
+   vb2_queue_enqueue_current_buffers(q);
 
/* Tell the driver to start streaming */
q->start_streaming_called = 1;
@@ -1361,7 +1379,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
return ret;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
+ struct media_request *req, void *pb)
 {
struct vb2_buffer *vb;
int ret;
@@ -1392,6 +1411,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
q->queued_count++;
q->waiting_for_buffers = false;
vb->state = VB2_BUF_STATE_QUEUED;
+   vb->request = req;
 
if (pb)
call_void_bufop(q, copy_timestamp, vb, pb);
@@ -1401,8 +1421,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
/*
 * If already streaming, give the buffer to driver for processing.
 * If not, the buffer will be given to driver on next streamon.
+*
+* If using the request API, the buffer will be given to the driver
+* when the request becomes active.
 */
-   if (q->start_streaming_called)
+   if (q->start_streaming_called && !req)
__enqueue_in_driver(vb);
 
/* Fill buffer information for the userspace */
@@ -1427,6 +1450,28 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
 
+void vb2_queue_start_request(struct vb2_queue *q, struct media_request *req)
+{
+   struct vb2_buffer *vb;
+
+   q->req_buf_cnt = 0;
+   list_for_each_entry(vb, >queued_list, queued_entry) {
+   if (vb->request == req)
+   ++q->req_buf_cnt;
+   }
+
+   /* only consider the request if we actually have buffers for it */
+   if (q->req_buf_cnt == 0)
+   return;
+
+   q->cur_req = req;
+
+   /* If not streaming yet, we will enqueue the buffers later */
+   if (q->start_streaming_called)
+   vb2_queue_enqueue_current_buffers(q);
+}
+EXPORT_SYMBOL_GPL(vb2_queue_start_request);
+
 /**
  * __vb2_wait_for_done_vb() - wait for a buffer to become available
  * for dequeuing
@@ -2242,7 +2287,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

Re: [RFC PATCH 5/9] media: vb2: add support for requests

[RFC PATCH 5/9] media: vb2: add support for requests

8 matches

Site Navigation

Mail list logo

Footer information