| Message ID | 1409480361-12821-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; >> > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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; >>> >> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)