Re: [RFCv11 PATCH 20/29] videobuf2-v4l2: integrate with media requests
On 04/17/2018 06:36 AM, Alexandre Courbot wrote:
> On Mon, Apr 9, 2018 at 11:21 PM Hans Verkuil wrote:
>
>> From: Hans Verkuil
>
>> This implements the V4L2 part of the request support. The main
>> change is that vb2_qbuf and vb2_prepare_buf now have a new
>> media_device pointer. This required changes to several drivers
>> that did not use the vb2_ioctl_qbuf/prepare_buf helper functions.
>
>> Signed-off-by: Hans Verkuil
>> ---
>> drivers/media/common/videobuf2/videobuf2-v4l2.c | 84
>
>> drivers/media/platform/omap3isp/ispvideo.c | 2 +-
>> drivers/media/platform/s3c-camif/camif-capture.c | 4 +-
>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 4 +-
>> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 4 +-
>> drivers/media/platform/soc_camera/soc_camera.c | 4 +-
>> drivers/media/usb/uvc/uvc_queue.c| 5 +-
>> drivers/media/usb/uvc/uvc_v4l2.c | 3 +-
>> drivers/media/usb/uvc/uvcvideo.h | 1 +
>> drivers/media/v4l2-core/v4l2-mem2mem.c | 7 +-
>> drivers/staging/media/davinci_vpfe/vpfe_video.c | 3 +-
>> drivers/staging/media/omap4iss/iss_video.c | 3 +-
>> drivers/usb/gadget/function/uvc_queue.c | 2 +-
>> include/media/videobuf2-v4l2.h | 12 +++-
>> 14 files changed, 106 insertions(+), 32 deletions(-)
>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index b8d370b97cca..73c1fd4da58a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -25,6 +25,7 @@
>> #include
>
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -40,10 +41,12 @@ module_param(debug, int, 0644);
>> pr_info("vb2-v4l2: %s: " fmt, __func__, ## arg); \
>> } while (0)
>
>> -/* Flags that are set by the vb2 core */
>> +/* Flags that are set by us */
>> #define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED |
> V4L2_BUF_FLAG_QUEUED | \
>> V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR
> | \
>> V4L2_BUF_FLAG_PREPARED | \
>> +V4L2_BUF_FLAG_IN_REQUEST | \
>> +V4L2_BUF_FLAG_REQUEST_FD | \
>> V4L2_BUF_FLAG_TIMESTAMP_MASK)
>> /* Output buffer flags that should be passed on to the driver */
>> #define V4L2_BUFFER_OUT_FLAGS (V4L2_BUF_FLAG_PFRAME |
> V4L2_BUF_FLAG_BFRAME | \
>> @@ -318,13 +321,17 @@ static int vb2_fill_vb2_v4l2_buffer(struct
> vb2_buffer *vb, struct v4l2_buffer *b
>> return 0;
>> }
>
>> -static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct
> v4l2_buffer *b,
>> - const char *opname)
>> +static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct
> media_device *mdev,
>> + struct v4l2_buffer *b,
>> + const char *opname,
>> + struct media_request **p_req)
>> {
>> + struct media_request *req;
>> struct vb2_v4l2_buffer *vbuf;
>> struct vb2_buffer *vb;
>> int ret;
>
>> + *p_req = NULL;
>> if (b->type != q->type) {
>> dprintk(1, "%s: invalid buffer type\n", opname);
>> return -EINVAL;
>> @@ -354,7 +361,38 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue
> *q, struct v4l2_buffer *b,
>
>> /* Copy relevant information provided by the userspace */
>> memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
>> - return vb2_fill_vb2_v4l2_buffer(vb, b);
>> + ret = vb2_fill_vb2_v4l2_buffer(vb, b);
>> + if (ret)
>> + return ret;
>> +
>> + if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD))
>> + return 0;
>> +
>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> + dprintk(1, "%s: buffer is not in dequeued state\n",
> opname);
>> + return -EINVAL;
>> + }
>> +
>> + if (b->request_fd < 0) {
>> + dprintk(1, "%s: request_fd < 0\n", opname);
>> + return -EINVAL;
>> + }
>> +
>> + req = media_request_find(mdev, b->request_fd);
>> + if (IS_ERR(req)) {
>> + dprintk(1, "%s: invalid request_fd\n", opname);
>> + return PTR_ERR(req);
>> + }
>> +
>> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
>> + dprintk(1, "%s: request is not idle\n", opname);
>> + media_request_put(req);
>> + return -EBUSY;
>> + }
>> +
>> + *p_req = req;
>> +
>> + return 0;
>> }
>
>> /*
>> @@ -437,6 +475,9 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
> void *pb)
>> case VB2_BUF_STATE_ACTIVE:
>> b->flags |= V4L2
Re: [RFCv11 PATCH 20/29] videobuf2-v4l2: integrate with media requests
On Mon, Apr 9, 2018 at 11:21 PM Hans Verkuil wrote:
> From: Hans Verkuil
> This implements the V4L2 part of the request support. The main
> change is that vb2_qbuf and vb2_prepare_buf now have a new
> media_device pointer. This required changes to several drivers
> that did not use the vb2_ioctl_qbuf/prepare_buf helper functions.
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/common/videobuf2/videobuf2-v4l2.c | 84
> drivers/media/platform/omap3isp/ispvideo.c | 2 +-
> drivers/media/platform/s3c-camif/camif-capture.c | 4 +-
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 4 +-
> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 4 +-
> drivers/media/platform/soc_camera/soc_camera.c | 4 +-
> drivers/media/usb/uvc/uvc_queue.c| 5 +-
> drivers/media/usb/uvc/uvc_v4l2.c | 3 +-
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> drivers/media/v4l2-core/v4l2-mem2mem.c | 7 +-
> drivers/staging/media/davinci_vpfe/vpfe_video.c | 3 +-
> drivers/staging/media/omap4iss/iss_video.c | 3 +-
> drivers/usb/gadget/function/uvc_queue.c | 2 +-
> include/media/videobuf2-v4l2.h | 12 +++-
> 14 files changed, 106 insertions(+), 32 deletions(-)
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index b8d370b97cca..73c1fd4da58a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -25,6 +25,7 @@
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -40,10 +41,12 @@ module_param(debug, int, 0644);
> pr_info("vb2-v4l2: %s: " fmt, __func__, ## arg); \
> } while (0)
> -/* Flags that are set by the vb2 core */
> +/* Flags that are set by us */
> #define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED |
V4L2_BUF_FLAG_QUEUED | \
> V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR
| \
> V4L2_BUF_FLAG_PREPARED | \
> +V4L2_BUF_FLAG_IN_REQUEST | \
> +V4L2_BUF_FLAG_REQUEST_FD | \
> V4L2_BUF_FLAG_TIMESTAMP_MASK)
> /* Output buffer flags that should be passed on to the driver */
> #define V4L2_BUFFER_OUT_FLAGS (V4L2_BUF_FLAG_PFRAME |
V4L2_BUF_FLAG_BFRAME | \
> @@ -318,13 +321,17 @@ static int vb2_fill_vb2_v4l2_buffer(struct
vb2_buffer *vb, struct v4l2_buffer *b
> return 0;
> }
> -static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct
v4l2_buffer *b,
> - const char *opname)
> +static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct
media_device *mdev,
> + struct v4l2_buffer *b,
> + const char *opname,
> + struct media_request **p_req)
> {
> + struct media_request *req;
> struct vb2_v4l2_buffer *vbuf;
> struct vb2_buffer *vb;
> int ret;
> + *p_req = NULL;
> if (b->type != q->type) {
> dprintk(1, "%s: invalid buffer type\n", opname);
> return -EINVAL;
> @@ -354,7 +361,38 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue
*q, struct v4l2_buffer *b,
> /* Copy relevant information provided by the userspace */
> memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
> - return vb2_fill_vb2_v4l2_buffer(vb, b);
> + ret = vb2_fill_vb2_v4l2_buffer(vb, b);
> + if (ret)
> + return ret;
> +
> + if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD))
> + return 0;
> +
> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> + dprintk(1, "%s: buffer is not in dequeued state\n",
opname);
> + return -EINVAL;
> + }
> +
> + if (b->request_fd < 0) {
> + dprintk(1, "%s: request_fd < 0\n", opname);
> + return -EINVAL;
> + }
> +
> + req = media_request_find(mdev, b->request_fd);
> + if (IS_ERR(req)) {
> + dprintk(1, "%s: invalid request_fd\n", opname);
> + return PTR_ERR(req);
> + }
> +
> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> + dprintk(1, "%s: request is not idle\n", opname);
> + media_request_put(req);
> + return -EBUSY;
> + }
> +
> + *p_req = req;
> +
> + return 0;
> }
> /*
> @@ -437,6 +475,9 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
void *pb)
> case VB2_BUF_STATE_ACTIVE:
> b->flags |= V4L2_BUF_FLAG_QUEUED;
> break;
> + case VB2_BUF_STATE_IN_REQUEST:
> + b->flags |= V4L2_BUF_FLAG_IN_REQUEST;
> + break;
> case VB2_BUF_STATE_ER
Re: [RFCv11 PATCH 20/29] videobuf2-v4l2: integrate with media requests
Em Mon, 9 Apr 2018 16:20:17 +0200
Hans Verkuil escreveu:
> From: Hans Verkuil
>
> This implements the V4L2 part of the request support. The main
> change is that vb2_qbuf and vb2_prepare_buf now have a new
> media_device pointer. This required changes to several drivers
> that did not use the vb2_ioctl_qbuf/prepare_buf helper functions.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/common/videobuf2/videobuf2-v4l2.c | 84
>
> drivers/media/platform/omap3isp/ispvideo.c | 2 +-
> drivers/media/platform/s3c-camif/camif-capture.c | 4 +-
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 4 +-
> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 4 +-
> drivers/media/platform/soc_camera/soc_camera.c | 4 +-
> drivers/media/usb/uvc/uvc_queue.c| 5 +-
> drivers/media/usb/uvc/uvc_v4l2.c | 3 +-
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> drivers/media/v4l2-core/v4l2-mem2mem.c | 7 +-
> drivers/staging/media/davinci_vpfe/vpfe_video.c | 3 +-
> drivers/staging/media/omap4iss/iss_video.c | 3 +-
> drivers/usb/gadget/function/uvc_queue.c | 2 +-
> include/media/videobuf2-v4l2.h | 12 +++-
> 14 files changed, 106 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index b8d370b97cca..73c1fd4da58a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -25,6 +25,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -40,10 +41,12 @@ module_param(debug, int, 0644);
> pr_info("vb2-v4l2: %s: " fmt, __func__, ## arg); \
> } while (0)
>
> -/* Flags that are set by the vb2 core */
> +/* Flags that are set by us */
> #define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED |
> V4L2_BUF_FLAG_QUEUED | \
>V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
>V4L2_BUF_FLAG_PREPARED | \
> + V4L2_BUF_FLAG_IN_REQUEST | \
> + V4L2_BUF_FLAG_REQUEST_FD | \
>V4L2_BUF_FLAG_TIMESTAMP_MASK)
> /* Output buffer flags that should be passed on to the driver */
> #define V4L2_BUFFER_OUT_FLAGS(V4L2_BUF_FLAG_PFRAME |
> V4L2_BUF_FLAG_BFRAME | \
> @@ -318,13 +321,17 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer
> *vb, struct v4l2_buffer *b
> return 0;
> }
>
> -static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer
> *b,
> - const char *opname)
> +static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device
> *mdev,
> + struct v4l2_buffer *b,
> + const char *opname,
> + struct media_request **p_req)
> {
> + struct media_request *req;
> struct vb2_v4l2_buffer *vbuf;
> struct vb2_buffer *vb;
> int ret;
>
> + *p_req = NULL;
> if (b->type != q->type) {
> dprintk(1, "%s: invalid buffer type\n", opname);
> return -EINVAL;
> @@ -354,7 +361,38 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q,
> struct v4l2_buffer *b,
>
> /* Copy relevant information provided by the userspace */
> memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
> - return vb2_fill_vb2_v4l2_buffer(vb, b);
> + ret = vb2_fill_vb2_v4l2_buffer(vb, b);
> + if (ret)
> + return ret;
> +
> + if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD))
> + return 0;
> +
> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> + dprintk(1, "%s: buffer is not in dequeued state\n", opname);
> + return -EINVAL;
> + }
> +
> + if (b->request_fd < 0) {
> + dprintk(1, "%s: request_fd < 0\n", opname);
> + return -EINVAL;
> + }
> +
> + req = media_request_find(mdev, b->request_fd);
> + if (IS_ERR(req)) {
> + dprintk(1, "%s: invalid request_fd\n", opname);
> + return PTR_ERR(req);
> + }
> +
> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> + dprintk(1, "%s: request is not idle\n", opname);
> + media_request_put(req);
> + return -EBUSY;
> + }
It is accessing req->state without locking. As I said before,
IMHO, it should really be an atomic var.
Also, on the changes introduced in this patch, it should be
doing the proper lock before using req (The way it is, it should
be locking the mutex - except that it won't work at IRQ),
or the entire logic should switch to some other locking type
or to use something like RCU.
> +
> + *p_req = req;
> +
> + return 0;
> }
>
> /*
> @@ -437,6 +475,9 @@ static void __fill_v4l2_bu
[RFCv11 PATCH 20/29] videobuf2-v4l2: integrate with media requests
From: Hans Verkuil
This implements the V4L2 part of the request support. The main
change is that vb2_qbuf and vb2_prepare_buf now have a new
media_device pointer. This required changes to several drivers
that did not use the vb2_ioctl_qbuf/prepare_buf helper functions.
Signed-off-by: Hans Verkuil
---
drivers/media/common/videobuf2/videobuf2-v4l2.c | 84
drivers/media/platform/omap3isp/ispvideo.c | 2 +-
drivers/media/platform/s3c-camif/camif-capture.c | 4 +-
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 4 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 4 +-
drivers/media/platform/soc_camera/soc_camera.c | 4 +-
drivers/media/usb/uvc/uvc_queue.c| 5 +-
drivers/media/usb/uvc/uvc_v4l2.c | 3 +-
drivers/media/usb/uvc/uvcvideo.h | 1 +
drivers/media/v4l2-core/v4l2-mem2mem.c | 7 +-
drivers/staging/media/davinci_vpfe/vpfe_video.c | 3 +-
drivers/staging/media/omap4iss/iss_video.c | 3 +-
drivers/usb/gadget/function/uvc_queue.c | 2 +-
include/media/videobuf2-v4l2.h | 12 +++-
14 files changed, 106 insertions(+), 32 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index b8d370b97cca..73c1fd4da58a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -25,6 +25,7 @@
#include
#include
+#include
#include
#include
#include
@@ -40,10 +41,12 @@ module_param(debug, int, 0644);
pr_info("vb2-v4l2: %s: " fmt, __func__, ## arg); \
} while (0)
-/* Flags that are set by the vb2 core */
+/* Flags that are set by us */
#define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
V4L2_BUF_FLAG_PREPARED | \
+V4L2_BUF_FLAG_IN_REQUEST | \
+V4L2_BUF_FLAG_REQUEST_FD | \
V4L2_BUF_FLAG_TIMESTAMP_MASK)
/* Output buffer flags that should be passed on to the driver */
#define V4L2_BUFFER_OUT_FLAGS (V4L2_BUF_FLAG_PFRAME | V4L2_BUF_FLAG_BFRAME | \
@@ -318,13 +321,17 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer
*vb, struct v4l2_buffer *b
return 0;
}
-static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
- const char *opname)
+static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device
*mdev,
+ struct v4l2_buffer *b,
+ const char *opname,
+ struct media_request **p_req)
{
+ struct media_request *req;
struct vb2_v4l2_buffer *vbuf;
struct vb2_buffer *vb;
int ret;
+ *p_req = NULL;
if (b->type != q->type) {
dprintk(1, "%s: invalid buffer type\n", opname);
return -EINVAL;
@@ -354,7 +361,38 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q,
struct v4l2_buffer *b,
/* Copy relevant information provided by the userspace */
memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
- return vb2_fill_vb2_v4l2_buffer(vb, b);
+ ret = vb2_fill_vb2_v4l2_buffer(vb, b);
+ if (ret)
+ return ret;
+
+ if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD))
+ return 0;
+
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ dprintk(1, "%s: buffer is not in dequeued state\n", opname);
+ return -EINVAL;
+ }
+
+ if (b->request_fd < 0) {
+ dprintk(1, "%s: request_fd < 0\n", opname);
+ return -EINVAL;
+ }
+
+ req = media_request_find(mdev, b->request_fd);
+ if (IS_ERR(req)) {
+ dprintk(1, "%s: invalid request_fd\n", opname);
+ return PTR_ERR(req);
+ }
+
+ if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+ dprintk(1, "%s: request is not idle\n", opname);
+ media_request_put(req);
+ return -EBUSY;
+ }
+
+ *p_req = req;
+
+ return 0;
}
/*
@@ -437,6 +475,9 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void
*pb)
case VB2_BUF_STATE_ACTIVE:
b->flags |= V4L2_BUF_FLAG_QUEUED;
break;
+ case VB2_BUF_STATE_IN_REQUEST:
+ b->flags |= V4L2_BUF_FLAG_IN_REQUEST;
+ break;
case VB2_BUF_STATE_ERROR:
b->flags |= V4L2_BUF_FLAG_ERROR;
/* fall through */
@@ -455,6 +496,10 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void
*pb)
if (vb2_buffer_in_use(q, vb))
b->flags |= V4L2_BUF_FLAG_MAPPED;
+ if (vb->req_obj.req) {
+
