[PATCH] videobuf: Allow reqbufs(0) to free current buffers

[PATCH] videobuf: Allow reqbufs(0) to free current buffers

[Hans de Goede]

All the infrastructure for this is already there, and despite our desires for
the old videobuf code to go away, it is currently still in use in 18 drivers.

Allowing reqbufs(0) makes these drivers behave consistent with modern drivers,
making live easier for userspace, see e.g. :
https://bugzilla.gnome.org/show_bug.cgi?id=735660

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/videobuf-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
index fb5ee5d..b91a266 100644
--- a/drivers/media/v4l2-core/videobuf-core.c
+++ b/drivers/media/v4l2-core/videobuf-core.c
@@ -441,11 +441,6 @@ int videobuf_reqbufs(struct videobuf_queue *q,
 	unsigned int size, count;
 	int retval;
 
-	if (req->count < 1) {
-		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
-		return -EINVAL;
-	}
-
 	if (req->memory != V4L2_MEMORY_MMAP     &&
 	    req->memory != V4L2_MEMORY_USERPTR  &&
 	    req->memory != V4L2_MEMORY_OVERLAY) {
@@ -471,6 +466,12 @@ int videobuf_reqbufs(struct videobuf_queue *q,
 		goto done;
 	}
 
+	if (req->count == 0) {
+		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
+		retval = __videobuf_free(q);
+		goto done;
+	}
+
 	count = req->count;
 	if (count > VIDEO_MAX_FRAME)
 		count = VIDEO_MAX_FRAME;
-- 
2.1.0

Re: [PATCH] videobuf: Allow reqbufs(0) to free current buffers

[Hans Verkuil]

Hi Hans,

At first glance this looks fine. But making changes in videobuf is always scary :-)
so I hope Marek can look at this as well.

How well was this tested?

I'll try do test this as well.

Regards,

	Hans

On 08/31/2014 12:19 PM, Hans de Goede wrote:
> All the infrastructure for this is already there, and despite our desires for
> the old videobuf code to go away, it is currently still in use in 18 drivers.
> 
> Allowing reqbufs(0) makes these drivers behave consistent with modern drivers,
> making live easier for userspace, see e.g. :
> https://bugzilla.gnome.org/show_bug.cgi?id=735660
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/v4l2-core/videobuf-core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
> index fb5ee5d..b91a266 100644
> --- a/drivers/media/v4l2-core/videobuf-core.c
> +++ b/drivers/media/v4l2-core/videobuf-core.c
> @@ -441,11 +441,6 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>  	unsigned int size, count;
>  	int retval;
>  
> -	if (req->count < 1) {
> -		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
> -		return -EINVAL;
> -	}
> -
>  	if (req->memory != V4L2_MEMORY_MMAP     &&
>  	    req->memory != V4L2_MEMORY_USERPTR  &&
>  	    req->memory != V4L2_MEMORY_OVERLAY) {
> @@ -471,6 +466,12 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>  		goto done;
>  	}
>  
> +	if (req->count == 0) {
> +		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
> +		retval = __videobuf_free(q);
> +		goto done;
> +	}
> +
>  	count = req->count;
>  	if (count > VIDEO_MAX_FRAME)
>  		count = VIDEO_MAX_FRAME;
> 

Re: [PATCH] videobuf: Allow reqbufs(0) to free current buffers

[Hans de Goede]

Hi,

On 09/01/2014 03:29 PM, Hans Verkuil wrote:
> Hi Hans,
> 
> At first glance this looks fine. But making changes in videobuf is always scary :-)
> so I hope Marek can look at this as well.
> 
> How well was this tested?

I ran some tests on bttv which all ran well.

Note that the code already allowed for going from say 4 buffers to 1,
and the old code path for reqbufs was already calling __videobuf_free()
before re-allocating the buffers again. So in essence this just changes
things to allow the 4 buffers to 1 case to also be 4 buffers to 0.

Regards,

Hans


> 
> I'll try do test this as well.
> 
> Regards,
> 
> 	Hans
> 
> On 08/31/2014 12:19 PM, Hans de Goede wrote:
>> All the infrastructure for this is already there, and despite our desires for
>> the old videobuf code to go away, it is currently still in use in 18 drivers.
>>
>> Allowing reqbufs(0) makes these drivers behave consistent with modern drivers,
>> making live easier for userspace, see e.g. :
>> https://bugzilla.gnome.org/show_bug.cgi?id=735660
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/v4l2-core/videobuf-core.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
>> index fb5ee5d..b91a266 100644
>> --- a/drivers/media/v4l2-core/videobuf-core.c
>> +++ b/drivers/media/v4l2-core/videobuf-core.c
>> @@ -441,11 +441,6 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>  	unsigned int size, count;
>>  	int retval;
>>  
>> -	if (req->count < 1) {
>> -		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>> -		return -EINVAL;
>> -	}
>> -
>>  	if (req->memory != V4L2_MEMORY_MMAP     &&
>>  	    req->memory != V4L2_MEMORY_USERPTR  &&
>>  	    req->memory != V4L2_MEMORY_OVERLAY) {
>> @@ -471,6 +466,12 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>  		goto done;
>>  	}
>>  
>> +	if (req->count == 0) {
>> +		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>> +		retval = __videobuf_free(q);
>> +		goto done;
>> +	}
>> +
>>  	count = req->count;
>>  	if (count > VIDEO_MAX_FRAME)
>>  		count = VIDEO_MAX_FRAME;
>>
> 

Re: [PATCH] videobuf: Allow reqbufs(0) to free current buffers

[Marek Szyprowski]

Hello,

On 2014-09-01 17:36, Hans de Goede wrote:
> Hi,
>
> On 09/01/2014 03:29 PM, Hans Verkuil wrote:
>> Hi Hans,
>>
>> At first glance this looks fine. But making changes in videobuf is always scary :-)
>> so I hope Marek can look at this as well.
>>
>> How well was this tested?
> I ran some tests on bttv which all ran well.
>
> Note that the code already allowed for going from say 4 buffers to 1,
> and the old code path for reqbufs was already calling __videobuf_free()
> before re-allocating the buffers again. So in essence this just changes
> things to allow the 4 buffers to 1 case to also be 4 buffers to 0.

The patch looks fine and the explanation is also convincing, so it 
turned out
that this long standing issue can be fixed quite easily, however I don't 
have
any hardware to do the tests right now.

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> On 08/31/2014 12:19 PM, Hans de Goede wrote:
>>> All the infrastructure for this is already there, and despite our desires for
>>> the old videobuf code to go away, it is currently still in use in 18 drivers.
>>>
>>> Allowing reqbufs(0) makes these drivers behave consistent with modern drivers,
>>> making live easier for userspace, see e.g. :
>>> https://bugzilla.gnome.org/show_bug.cgi?id=735660
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/media/v4l2-core/videobuf-core.c | 11 ++++++-----
>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
>>> index fb5ee5d..b91a266 100644
>>> --- a/drivers/media/v4l2-core/videobuf-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf-core.c
>>> @@ -441,11 +441,6 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>>   	unsigned int size, count;
>>>   	int retval;
>>>   
>>> -	if (req->count < 1) {
>>> -		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>>   	if (req->memory != V4L2_MEMORY_MMAP     &&
>>>   	    req->memory != V4L2_MEMORY_USERPTR  &&
>>>   	    req->memory != V4L2_MEMORY_OVERLAY) {
>>> @@ -471,6 +466,12 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>>   		goto done;
>>>   	}
>>>   
>>> +	if (req->count == 0) {
>>> +		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>>> +		retval = __videobuf_free(q);
>>> +		goto done;
>>> +	}
>>> +
>>>   	count = req->count;
>>>   	if (count > VIDEO_MAX_FRAME)
>>>   		count = VIDEO_MAX_FRAME;
>>>
>>>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] videobuf: Allow reqbufs(0) to free current buffers

[Hans Verkuil]

On 09/01/14 17:36, Hans de Goede wrote:
> Hi,
> 
> On 09/01/2014 03:29 PM, Hans Verkuil wrote:
>> Hi Hans,
>>
>> At first glance this looks fine. But making changes in videobuf is always scary :-)
>> so I hope Marek can look at this as well.
>>
>> How well was this tested?
> 
> I ran some tests on bttv which all ran well.
> 
> Note that the code already allowed for going from say 4 buffers to 1,
> and the old code path for reqbufs was already calling __videobuf_free()
> before re-allocating the buffers again. So in essence this just changes
> things to allow the 4 buffers to 1 case to also be 4 buffers to 0.

The code looks good and I've tested it with cx88 which worked (well, except
for the zillion other bugs in cx88, but that's another story). I do want to
do a few more tests on Friday, though. This time with a more stable driver.

I think we should consider adding this to the stable tree as well.

Regards,

	Hans

> 
> Regards,
> 
> Hans
> 
> 
>>
>> I'll try do test this as well.
>>
>> Regards,
>>
>> 	Hans
>>
>> On 08/31/2014 12:19 PM, Hans de Goede wrote:
>>> All the infrastructure for this is already there, and despite our desires for
>>> the old videobuf code to go away, it is currently still in use in 18 drivers.
>>>
>>> Allowing reqbufs(0) makes these drivers behave consistent with modern drivers,
>>> making live easier for userspace, see e.g. :
>>> https://bugzilla.gnome.org/show_bug.cgi?id=735660
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/media/v4l2-core/videobuf-core.c | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
>>> index fb5ee5d..b91a266 100644
>>> --- a/drivers/media/v4l2-core/videobuf-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf-core.c
>>> @@ -441,11 +441,6 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>>  	unsigned int size, count;
>>>  	int retval;
>>>  
>>> -	if (req->count < 1) {
>>> -		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>>  	if (req->memory != V4L2_MEMORY_MMAP     &&
>>>  	    req->memory != V4L2_MEMORY_USERPTR  &&
>>>  	    req->memory != V4L2_MEMORY_OVERLAY) {
>>> @@ -471,6 +466,12 @@ int videobuf_reqbufs(struct videobuf_queue *q,
>>>  		goto done;
>>>  	}
>>>  
>>> +	if (req->count == 0) {
>>> +		dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
>>> +		retval = __videobuf_free(q);
>>> +		goto done;
>>> +	}
>>> +
>>>  	count = req->count;
>>>  	if (count > VIDEO_MAX_FRAME)
>>>  		count = VIDEO_MAX_FRAME;
>>>
>>