Re: [PATCH] s2255drv: port to videobuf2

[Hans Verkuil <hverkuil@xs4all.nl>]

On 02/05/2014 10:44 PM, Dean Anderson wrote:
> On 2014-02-04 04:04, Hans Verkuil wrote:
>> Hi Dean,
>>
>> On 02/03/14 18:06, Dean Anderson wrote:
>>> On 2014-02-03 03:51, Hans Verkuil wrote:
>>>> Hi Dean,
>>>>
>>>> Some specific comments below, but first two general comments:
>>>>
>>>> It is easier to review if at least the removal of the old s2255_fh 
>>>> struct
>>>> was done as a separate patch. It's always good to try and keep the 
>>>> changes
>>>> in patches as small as possible. The actual vb2 conversion is always 
>>>> a
>>>> 'big bang' patch, that's unavoidable, but it's easier if it isn't 
>>>> mixed in
>>>> with other changes that are not directly related to the vb2 
>>>> conversion.
>>>
>>>
>>> I figured removal of s2255_fh was a natural part of the videobuf2 
>>> conversion process, but I can break it up.
>>
>> It's more like the first phase of a vb2 conversion. It really is wrong
>> for videobuf as well, so it makes sense to do that first.
>>
>>> I also did change some formatting and naming changes (s2255_channel 
>>> to s2255_vc) that can be postponed.
>>
>> Just put it in a separate patch either before or after the patch that 
>> does
>> the vb2 conversion.
>>
>>>
>>>>
>>>> And did you also run the v4l2-compliance utility for this driver? 
>>>> That's
>>>> useful to check that everything it still correct.
>>>
>>> Thanks for the comments.  I'll do a v2 soon with v4l2-compliance 
>>> fully tested too.
>>
>> Rather than the standard v4l2-compliance from v4l-utils, can you use 
>> this
>> from my own tree:
>>
>> http://git.linuxtv.org/hverkuil/v4l-utils.git/shortlog/refs/heads/streaming
>>
>> I've started work to add tests for streaming to v4l2-compliance. While 
>> not
>> complete it should cover what the s2255 driver needs. I'm very 
>> interested
>> in what it finds (or, as the case might be, what it doesn't find).
>>
>> In order to do the streaming tests you have to run it with option -s.
>>
> 
> The current driver before the videobuf2 patch has 3 errors and 8 
> warnings with option "-s".  The warnings are "msg5650" warnings that 
> will break existing applications if fixed.
> 
> Here's what is causing at least two of the errors (the other is 
> unsupported USERPTR, which will be fixed in VB2):
> 
> Vidioc_reqbufs calls videobuf_reqbufs, which returns a fail if 
> req->count = 0.
> 
> IE:
> 
> if (req->count < 1) {
> 	dprintk(1, "reqbufs: count invalid (%d)\n", req->count);
> 	return -EINVAL;
> }
> 
> Are drivers using videobuf required to check if 
> v4l2_requestbuffers->count == 0 before calling videobuf_reqbufs? That 
> seems unlikely and inefficient, so this could be an issue with 
> videobuf-core.c..

videobuf has always handled REQBUFS with a count of 0 wrong. One of the
many bad things about videobuf. Any driver using videobuf will fail the
v4l2-compliance test so that is not surprising.

Regards,

	Hans