* [PATCH v5.1 0/2] add requires_requests bit for stateless codecs @ 2019-03-20 12:33 hverkuil-cisco 2019-03-20 12:33 ` [PATCH v5.1 1/2] vb2: " hverkuil-cisco 2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco 0 siblings, 2 replies; 9+ messages in thread From: hverkuil-cisco @ 2019-03-20 12:33 UTC (permalink / raw) To: linux-media From: Hans Verkuil <hverkuil-cisco@xs4all.nl> These two patches replace patches 1-3 of the vicodec v5 patch series: https://www.spinics.net/lists/linux-media/msg147923.html The original patch 2/23 (videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS) was dropped. This needs a bit more thought. Patch 1 now returns EBADR if an attempt is made to queue a buffer without a request and requires_requests is set. This error code is also documented. Both patches have a better (hopefully) commit log. The cedrus patch itself is unchanged from v5. Regards, Hans Hans Verkuil (2): vb2: add requires_requests bit for stateless codecs cedrus: set requires_requests Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++ drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++ drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++ drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 + include/media/videobuf2-core.h | 3 +++ 5 files changed, 21 insertions(+) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs 2019-03-20 12:33 [PATCH v5.1 0/2] add requires_requests bit for stateless codecs hverkuil-cisco @ 2019-03-20 12:33 ` hverkuil-cisco 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco 1 sibling, 1 reply; 9+ messages in thread From: hverkuil-cisco @ 2019-03-20 12:33 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Paul Kocialkowski From: Hans Verkuil <hverkuil-cisco@xs4all.nl> Stateless codecs require the use of the Request API as opposed of it being optional. So add a bit to indicate this and let vb2 check for this. If an attempt is made to queue a buffer without an associated request, then the EBADR error is returned to userspace. Doing this check in the vb2 core simplifies drivers, since they don't have to check for this, they can just set this flag. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++ drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++ drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++ include/media/videobuf2-core.h | 3 +++ 4 files changed, 20 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst index c138d149faea..5739c3676062 100644 --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst @@ -189,6 +189,10 @@ EACCES The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not support requests for the given buffer type. +EBADR + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires + that the buffer is part of a request. + EBUSY The first buffer was queued via a request, but the application now tries to queue it directly, or vice versa (it is not permitted to mix the two diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 678a31a2b549..b98ec6e1a222 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -1507,6 +1507,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, vb = q->bufs[index]; + if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && + q->requires_requests) { + dprintk(1, "qbuf requires a request\n"); + return -EBADR; + } + if ((req && q->uses_qbuf) || (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && q->uses_requests)) { @@ -2238,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q) WARN_ON(!q->ops->buf_queue)) return -EINVAL; + if (WARN_ON(q->requires_requests && !q->supports_requests)) + return -EINVAL; + INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); spin_lock_init(&q->done_lock); diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 74d3abf33b50..84de18b30a95 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -381,6 +381,10 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md return 0; if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD)) { + if (q->requires_requests) { + dprintk(1, "%s: queue requires requests\n", opname); + return -EBADR; + } if (q->uses_requests) { dprintk(1, "%s: queue uses requests\n", opname); return -EBUSY; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index c02af6370e9b..fe010ad62b90 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -482,6 +482,8 @@ struct vb2_buf_ops { * has not been called. This is a vb1 idiom that has been adopted * also by vb2. * @supports_requests: this queue supports the Request API. + * @requires_requests: this queue requires the Request API. If this is set to 1, + * then supports_requests must be set to 1 as well. * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first * time this is called. Set to 0 when the queue is canceled. * If this is 1, then you cannot queue buffers from a request. @@ -556,6 +558,7 @@ struct vb2_queue { unsigned allow_zero_bytesused:1; unsigned quirk_poll_must_check_waiting_for_buffers:1; unsigned supports_requests:1; + unsigned requires_requests:1; unsigned uses_qbuf:1; unsigned uses_requests:1; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs 2019-03-20 12:33 ` [PATCH v5.1 1/2] vb2: " hverkuil-cisco @ 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 13:06 ` Hans Verkuil 0 siblings, 1 reply; 9+ messages in thread From: Mauro Carvalho Chehab @ 2019-03-20 12:55 UTC (permalink / raw) To: hverkuil-cisco; +Cc: linux-media, Paul Kocialkowski Em Wed, 20 Mar 2019 13:33:04 +0100 hverkuil-cisco@xs4all.nl escreveu: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Stateless codecs require the use of the Request API as opposed of it > being optional. > > So add a bit to indicate this and let vb2 check for this. > > If an attempt is made to queue a buffer without an associated request, > then the EBADR error is returned to userspace. > > Doing this check in the vb2 core simplifies drivers, since they > don't have to check for this, they can just set this flag. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++ > drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++ > drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++ > include/media/videobuf2-core.h | 3 +++ > 4 files changed, 20 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > index c138d149faea..5739c3676062 100644 > --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst > +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > @@ -189,6 +189,10 @@ EACCES > The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > support requests for the given buffer type. > > +EBADR > + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires > + that the buffer is part of a request. > + Hmm... IMO, you should replace the previous text instead: EACCES The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not support requests for the given buffer type. Also, I would replace: device -> device driver As this ia a device driver limitation of the current implementation, with may or may not reflect a hardware limitation. > EBUSY > The first buffer was queued via a request, but the application now tries > to queue it directly, or vice versa (it is not permitted to mix the two > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 678a31a2b549..b98ec6e1a222 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -1507,6 +1507,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, > > vb = q->bufs[index]; > > + if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && > + q->requires_requests) { > + dprintk(1, "qbuf requires a request\n"); > + return -EBADR; > + } > + > if ((req && q->uses_qbuf) || > (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && > q->uses_requests)) { > @@ -2238,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q) > WARN_ON(!q->ops->buf_queue)) > return -EINVAL; > > + if (WARN_ON(q->requires_requests && !q->supports_requests)) > + return -EINVAL; > + Shouldn't it also be EBADR? > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > spin_lock_init(&q->done_lock); > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 74d3abf33b50..84de18b30a95 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -381,6 +381,10 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > return 0; > > if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD)) { > + if (q->requires_requests) { > + dprintk(1, "%s: queue requires requests\n", opname); > + return -EBADR; > + } > if (q->uses_requests) { > dprintk(1, "%s: queue uses requests\n", opname); > return -EBUSY; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index c02af6370e9b..fe010ad62b90 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -482,6 +482,8 @@ struct vb2_buf_ops { > * has not been called. This is a vb1 idiom that has been adopted > * also by vb2. > * @supports_requests: this queue supports the Request API. > + * @requires_requests: this queue requires the Request API. If this is set to 1, > + * then supports_requests must be set to 1 as well. > * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first > * time this is called. Set to 0 when the queue is canceled. > * If this is 1, then you cannot queue buffers from a request. > @@ -556,6 +558,7 @@ struct vb2_queue { > unsigned allow_zero_bytesused:1; > unsigned quirk_poll_must_check_waiting_for_buffers:1; > unsigned supports_requests:1; > + unsigned requires_requests:1; > unsigned uses_qbuf:1; > unsigned uses_requests:1; > Thanks, Mauro ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs 2019-03-20 12:55 ` Mauro Carvalho Chehab @ 2019-03-20 13:06 ` Hans Verkuil 2019-03-20 13:22 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 9+ messages in thread From: Hans Verkuil @ 2019-03-20 13:06 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, Paul Kocialkowski On 3/20/19 1:55 PM, Mauro Carvalho Chehab wrote: > Em Wed, 20 Mar 2019 13:33:04 +0100 > hverkuil-cisco@xs4all.nl escreveu: > >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Stateless codecs require the use of the Request API as opposed of it >> being optional. >> >> So add a bit to indicate this and let vb2 check for this. >> >> If an attempt is made to queue a buffer without an associated request, >> then the EBADR error is returned to userspace. >> >> Doing this check in the vb2 core simplifies drivers, since they >> don't have to check for this, they can just set this flag. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >> --- >> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++ >> drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++ >> drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++ >> include/media/videobuf2-core.h | 3 +++ >> 4 files changed, 20 insertions(+) >> >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> index c138d149faea..5739c3676062 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> @@ -189,6 +189,10 @@ EACCES >> The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not >> support requests for the given buffer type. >> >> +EBADR >> + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires >> + that the buffer is part of a request. >> + > > Hmm... IMO, you should replace the previous text instead: > > EACCES > The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > support requests for the given buffer type. No. This is already being returned, so changing this will be an API change. That said, since the only drivers that can return this are vivid, vim2m and cedrus, (i.e. test and staging drivers), I am OK to change this to EBADR as well. In that case it would become: EBADR The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device driver does not support requests for the given buffer type, or the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device driver requires that the buffer is part of a request. > > Also, I would replace: > > device -> device driver > > As this ia a device driver limitation of the current implementation, > with may or may not reflect a hardware limitation. > >> EBUSY >> The first buffer was queued via a request, but the application now tries >> to queue it directly, or vice versa (it is not permitted to mix the two >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 678a31a2b549..b98ec6e1a222 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -1507,6 +1507,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, >> >> vb = q->bufs[index]; >> >> + if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && >> + q->requires_requests) { >> + dprintk(1, "qbuf requires a request\n"); >> + return -EBADR; >> + } >> + >> if ((req && q->uses_qbuf) || >> (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && >> q->uses_requests)) { >> @@ -2238,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q) >> WARN_ON(!q->ops->buf_queue)) >> return -EINVAL; >> >> + if (WARN_ON(q->requires_requests && !q->supports_requests)) >> + return -EINVAL; >> + > > Shouldn't it also be EBADR? No, this checks that the driver doesn't set requires_requests without also setting supports_requests. I.e. this indicates a driver bug, hence the WARN_ON. Requiring requests, but not supporting them makes obviously no sense. Regards, Hans > >> INIT_LIST_HEAD(&q->queued_list); >> INIT_LIST_HEAD(&q->done_list); >> spin_lock_init(&q->done_lock); >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index 74d3abf33b50..84de18b30a95 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -381,6 +381,10 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md >> return 0; >> >> if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD)) { >> + if (q->requires_requests) { >> + dprintk(1, "%s: queue requires requests\n", opname); >> + return -EBADR; >> + } >> if (q->uses_requests) { >> dprintk(1, "%s: queue uses requests\n", opname); >> return -EBUSY; >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index c02af6370e9b..fe010ad62b90 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -482,6 +482,8 @@ struct vb2_buf_ops { >> * has not been called. This is a vb1 idiom that has been adopted >> * also by vb2. >> * @supports_requests: this queue supports the Request API. >> + * @requires_requests: this queue requires the Request API. If this is set to 1, >> + * then supports_requests must be set to 1 as well. >> * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first >> * time this is called. Set to 0 when the queue is canceled. >> * If this is 1, then you cannot queue buffers from a request. >> @@ -556,6 +558,7 @@ struct vb2_queue { >> unsigned allow_zero_bytesused:1; >> unsigned quirk_poll_must_check_waiting_for_buffers:1; >> unsigned supports_requests:1; >> + unsigned requires_requests:1; >> unsigned uses_qbuf:1; >> unsigned uses_requests:1; >> > > > > Thanks, > Mauro > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs 2019-03-20 13:06 ` Hans Verkuil @ 2019-03-20 13:22 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 9+ messages in thread From: Mauro Carvalho Chehab @ 2019-03-20 13:22 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, Paul Kocialkowski Em Wed, 20 Mar 2019 14:06:51 +0100 Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu: > On 3/20/19 1:55 PM, Mauro Carvalho Chehab wrote: > > Em Wed, 20 Mar 2019 13:33:04 +0100 > > hverkuil-cisco@xs4all.nl escreveu: > > > >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >> Stateless codecs require the use of the Request API as opposed of it > >> being optional. > >> > >> So add a bit to indicate this and let vb2 check for this. > >> > >> If an attempt is made to queue a buffer without an associated request, > >> then the EBADR error is returned to userspace. > >> > >> Doing this check in the vb2 core simplifies drivers, since they > >> don't have to check for this, they can just set this flag. > >> > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > >> --- > >> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++ > >> drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++ > >> drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++ > >> include/media/videobuf2-core.h | 3 +++ > >> 4 files changed, 20 insertions(+) > >> > >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > >> index c138d149faea..5739c3676062 100644 > >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst > >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > >> @@ -189,6 +189,10 @@ EACCES > >> The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > >> support requests for the given buffer type. > >> > >> +EBADR > >> + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires > >> + that the buffer is part of a request. > >> + > > > > Hmm... IMO, you should replace the previous text instead: > > > > EACCES > > The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > > support requests for the given buffer type. > > No. This is already being returned, so changing this will be an API change. > > That said, since the only drivers that can return this are vivid, vim2m and cedrus, > (i.e. test and staging drivers), I am OK to change this to EBADR as well. > > In that case it would become: > > EBADR > The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device driver does > not support requests for the given buffer type, or the > ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device driver > requires that the buffer is part of a request. Ok, let's do that, as, IMHO, it makes it a lot more clear. > > > > > Also, I would replace: > > > > device -> device driver > > > > As this ia a device driver limitation of the current implementation, > > with may or may not reflect a hardware limitation. > > > >> EBUSY > >> The first buffer was queued via a request, but the application now tries > >> to queue it directly, or vice versa (it is not permitted to mix the two > >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >> index 678a31a2b549..b98ec6e1a222 100644 > >> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >> @@ -1507,6 +1507,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, > >> > >> vb = q->bufs[index]; > >> > >> + if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && > >> + q->requires_requests) { > >> + dprintk(1, "qbuf requires a request\n"); > >> + return -EBADR; > >> + } > >> + > >> if ((req && q->uses_qbuf) || > >> (!req && vb->state != VB2_BUF_STATE_IN_REQUEST && > >> q->uses_requests)) { > >> @@ -2238,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q) > >> WARN_ON(!q->ops->buf_queue)) > >> return -EINVAL; > >> > >> + if (WARN_ON(q->requires_requests && !q->supports_requests)) > >> + return -EINVAL; > >> + > > > > Shouldn't it also be EBADR? > > No, this checks that the driver doesn't set requires_requests without > also setting supports_requests. I.e. this indicates a driver bug, hence > the WARN_ON. Requiring requests, but not supporting them makes obviously > no sense. Ok. Thanks, Mauro ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5.1 2/2] cedrus: set requires_requests 2019-03-20 12:33 [PATCH v5.1 0/2] add requires_requests bit for stateless codecs hverkuil-cisco 2019-03-20 12:33 ` [PATCH v5.1 1/2] vb2: " hverkuil-cisco @ 2019-03-20 12:33 ` hverkuil-cisco 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 14:23 ` [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES Hans Verkuil 1 sibling, 2 replies; 9+ messages in thread From: hverkuil-cisco @ 2019-03-20 12:33 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Paul Kocialkowski From: Hans Verkuil <hverkuil-cisco@xs4all.nl> The cedrus stateless decoder requires the use of request, so indicate this by setting requires_requests to 1. Note that the cedrus driver never checked for this, and as far as I can tell would just crash if an attempt was made to queue a buffer without a request. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c index b47854b3bce4..9673874ece10 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c @@ -536,6 +536,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq, src_vq->lock = &ctx->dev->dev_mutex; src_vq->dev = ctx->dev->dev; src_vq->supports_requests = true; + src_vq->requires_requests = true; ret = vb2_queue_init(src_vq); if (ret) -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5.1 2/2] cedrus: set requires_requests 2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco @ 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 14:23 ` [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES Hans Verkuil 1 sibling, 0 replies; 9+ messages in thread From: Mauro Carvalho Chehab @ 2019-03-20 12:55 UTC (permalink / raw) To: hverkuil-cisco; +Cc: linux-media, Paul Kocialkowski Em Wed, 20 Mar 2019 13:33:05 +0100 hverkuil-cisco@xs4all.nl escreveu: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > The cedrus stateless decoder requires the use of request, so > indicate this by setting requires_requests to 1. > > Note that the cedrus driver never checked for this, and as far > as I can tell would just crash if an attempt was made to queue > a buffer without a request. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > index b47854b3bce4..9673874ece10 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > @@ -536,6 +536,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq, > src_vq->lock = &ctx->dev->dev_mutex; > src_vq->dev = ctx->dev->dev; > src_vq->supports_requests = true; > + src_vq->requires_requests = true; looks OK to my eyes. > > ret = vb2_queue_init(src_vq); > if (ret) Thanks, Mauro ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES 2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco 2019-03-20 12:55 ` Mauro Carvalho Chehab @ 2019-03-20 14:23 ` Hans Verkuil 2019-03-20 14:28 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 9+ messages in thread From: Hans Verkuil @ 2019-03-20 14:23 UTC (permalink / raw) To: linux-media; +Cc: Paul Kocialkowski If requests are used when they shouldn't, or not used when they should, then return EBADR (Invalid request descriptor) instead of EACCES. The reason for this change is that EACCES has more to do with permissions (not being the owner of the resource), but in this case the request file descriptor is just wrong for the current mode of the device. Update the documentation accordingly. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst index 1ad631e549fe..a74c82d95609 100644 --- a/Documentation/media/uapi/mediactl/request-api.rst +++ b/Documentation/media/uapi/mediactl/request-api.rst @@ -93,7 +93,7 @@ A queued request cannot be modified anymore. .. caution:: For :ref:`memory-to-memory devices <mem2mem>` you can use requests only for output buffers, not for capture buffers. Attempting to add a capture buffer - to a request will result in an ``EACCES`` error. + to a request will result in an ``EBADR`` error. If the request contains configurations for multiple entities, individual drivers may synchronize so the requested pipeline's topology is applied before the diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst index 81ffdcb89057..b9a2e9dc707c 100644 --- a/Documentation/media/uapi/v4l/buffer.rst +++ b/Documentation/media/uapi/v4l/buffer.rst @@ -326,7 +326,7 @@ struct v4l2_buffer Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`. - If the device does not support requests, then ``EACCES`` will be returned. + If the device does not support requests, then ``EBADR`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst index 5739c3676062..dbf7b445a27b 100644 --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst @@ -111,7 +111,7 @@ in use. Setting it means that the buffer will not be passed to the driver until the request itself is queued. Also, the driver will apply any settings associated with the request for this buffer. This field will be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set. -If the device does not support requests, then ``EACCES`` will be returned. +If the device does not support requests, then ``EBADR`` will be returned. If requests are supported but an invalid request file descriptor is given, then ``EINVAL`` will be returned. @@ -125,7 +125,7 @@ then ``EINVAL`` will be returned. For :ref:`memory-to-memory devices <mem2mem>` you can specify the ``request_fd`` only for output buffers, not for capture buffers. Attempting - to specify this for a capture buffer will result in an ``EACCES`` error. + to specify this for a capture buffer will result in an ``EBADR`` error. Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled (capturing) or displayed (output) buffer from the driver's outgoing @@ -185,12 +185,10 @@ EPIPE codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already dequeued and no new buffers are expected to become available. -EACCES - The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not - support requests for the given buffer type. - EBADR - The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not + support requests for the given buffer type, or + the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires that the buffer is part of a request. EBUSY diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 84de18b30a95..b11a779e97b0 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -392,7 +392,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md return 0; } else if (!q->supports_requests) { dprintk(1, "%s: queue does not support requests\n", opname); - return -EACCES; + return -EBADR; } else if (q->uses_qbuf) { dprintk(1, "%s: queue does not use requests\n", opname); return -EBUSY; diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c index eec2e2b2f6ec..ed87305b29f9 100644 --- a/drivers/media/media-request.c +++ b/drivers/media/media-request.c @@ -251,7 +251,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd) if (!mdev || !mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue) - return ERR_PTR(-EACCES); + return ERR_PTR(-EBADR); filp = fget(request_fd); if (!filp) @@ -407,7 +407,7 @@ int media_request_object_bind(struct media_request *req, int ret = -EBUSY; if (WARN_ON(!ops->release)) - return -EACCES; + return -EBADR; spin_lock_irqsave(&req->lock, flags); diff --git a/include/media/media-request.h b/include/media/media-request.h index bd36d7431698..3cd25a2717ce 100644 --- a/include/media/media-request.h +++ b/include/media/media-request.h @@ -198,7 +198,7 @@ void media_request_put(struct media_request *req); * Get the request represented by @request_fd that is owned * by the media device. * - * Return a -EACCES error pointer if requests are not supported + * Return a -EBADR error pointer if requests are not supported * by this driver. Return -EINVAL if the request was not found. * Return the pointer to the request if found: the caller will * have to call @media_request_put when it finished using the @@ -231,7 +231,7 @@ static inline void media_request_put(struct media_request *req) static inline struct media_request * media_request_get_by_fd(struct media_device *mdev, int request_fd) { - return ERR_PTR(-EACCES); + return ERR_PTR(-EBADR); } #endif ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES 2019-03-20 14:23 ` [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES Hans Verkuil @ 2019-03-20 14:28 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 9+ messages in thread From: Mauro Carvalho Chehab @ 2019-03-20 14:28 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, Paul Kocialkowski Em Wed, 20 Mar 2019 15:23:07 +0100 Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu: > If requests are used when they shouldn't, or not used when they should, then > return EBADR (Invalid request descriptor) instead of EACCES. > > The reason for this change is that EACCES has more to do with permissions > (not being the owner of the resource), but in this case the request file > descriptor is just wrong for the current mode of the device. > > Update the documentation accordingly. Looks OK to me. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst > index 1ad631e549fe..a74c82d95609 100644 > --- a/Documentation/media/uapi/mediactl/request-api.rst > +++ b/Documentation/media/uapi/mediactl/request-api.rst > @@ -93,7 +93,7 @@ A queued request cannot be modified anymore. > .. caution:: > For :ref:`memory-to-memory devices <mem2mem>` you can use requests only for > output buffers, not for capture buffers. Attempting to add a capture buffer > - to a request will result in an ``EACCES`` error. > + to a request will result in an ``EBADR`` error. > > If the request contains configurations for multiple entities, individual drivers > may synchronize so the requested pipeline's topology is applied before the > diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst > index 81ffdcb89057..b9a2e9dc707c 100644 > --- a/Documentation/media/uapi/v4l/buffer.rst > +++ b/Documentation/media/uapi/v4l/buffer.rst > @@ -326,7 +326,7 @@ struct v4l2_buffer > Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls > other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`. > > - If the device does not support requests, then ``EACCES`` will be returned. > + If the device does not support requests, then ``EBADR`` will be returned. > If requests are supported but an invalid request file descriptor is > given, then ``EINVAL`` will be returned. > > diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > index 5739c3676062..dbf7b445a27b 100644 > --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst > +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > @@ -111,7 +111,7 @@ in use. Setting it means that the buffer will not be passed to the driver > until the request itself is queued. Also, the driver will apply any > settings associated with the request for this buffer. This field will > be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set. > -If the device does not support requests, then ``EACCES`` will be returned. > +If the device does not support requests, then ``EBADR`` will be returned. > If requests are supported but an invalid request file descriptor is given, > then ``EINVAL`` will be returned. > > @@ -125,7 +125,7 @@ then ``EINVAL`` will be returned. > > For :ref:`memory-to-memory devices <mem2mem>` you can specify the > ``request_fd`` only for output buffers, not for capture buffers. Attempting > - to specify this for a capture buffer will result in an ``EACCES`` error. > + to specify this for a capture buffer will result in an ``EBADR`` error. > > Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled > (capturing) or displayed (output) buffer from the driver's outgoing > @@ -185,12 +185,10 @@ EPIPE > codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already > dequeued and no new buffers are expected to become available. > > -EACCES > - The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > - support requests for the given buffer type. > - > EBADR > - The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires > + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not > + support requests for the given buffer type, or > + the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires > that the buffer is part of a request. > > EBUSY > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 84de18b30a95..b11a779e97b0 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -392,7 +392,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > return 0; > } else if (!q->supports_requests) { > dprintk(1, "%s: queue does not support requests\n", opname); > - return -EACCES; > + return -EBADR; > } else if (q->uses_qbuf) { > dprintk(1, "%s: queue does not use requests\n", opname); > return -EBUSY; > diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c > index eec2e2b2f6ec..ed87305b29f9 100644 > --- a/drivers/media/media-request.c > +++ b/drivers/media/media-request.c > @@ -251,7 +251,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd) > > if (!mdev || !mdev->ops || > !mdev->ops->req_validate || !mdev->ops->req_queue) > - return ERR_PTR(-EACCES); > + return ERR_PTR(-EBADR); > > filp = fget(request_fd); > if (!filp) > @@ -407,7 +407,7 @@ int media_request_object_bind(struct media_request *req, > int ret = -EBUSY; > > if (WARN_ON(!ops->release)) > - return -EACCES; > + return -EBADR; > > spin_lock_irqsave(&req->lock, flags); > > diff --git a/include/media/media-request.h b/include/media/media-request.h > index bd36d7431698..3cd25a2717ce 100644 > --- a/include/media/media-request.h > +++ b/include/media/media-request.h > @@ -198,7 +198,7 @@ void media_request_put(struct media_request *req); > * Get the request represented by @request_fd that is owned > * by the media device. > * > - * Return a -EACCES error pointer if requests are not supported > + * Return a -EBADR error pointer if requests are not supported > * by this driver. Return -EINVAL if the request was not found. > * Return the pointer to the request if found: the caller will > * have to call @media_request_put when it finished using the > @@ -231,7 +231,7 @@ static inline void media_request_put(struct media_request *req) > static inline struct media_request * > media_request_get_by_fd(struct media_device *mdev, int request_fd) > { > - return ERR_PTR(-EACCES); > + return ERR_PTR(-EBADR); > } > > #endif Thanks, Mauro ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-20 14:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-20 12:33 [PATCH v5.1 0/2] add requires_requests bit for stateless codecs hverkuil-cisco 2019-03-20 12:33 ` [PATCH v5.1 1/2] vb2: " hverkuil-cisco 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 13:06 ` Hans Verkuil 2019-03-20 13:22 ` Mauro Carvalho Chehab 2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco 2019-03-20 12:55 ` Mauro Carvalho Chehab 2019-03-20 14:23 ` [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES Hans Verkuil 2019-03-20 14:28 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).