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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
