* [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
@ 2019-04-30 8:31 Hans Verkuil
2019-05-13 9:36 ` Hans Verkuil
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2019-04-30 8:31 UTC (permalink / raw)
To: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa,
Nicolas Dufresne, Alexandre Courbot, Boris Brezillon,
Maxime Ripard, Thierry Reding
This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
a v4l2_m2m_release_capture_buf() helper function.
Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
to indicate support for this flag.
At the start of the device_run function drivers should first check if the
current capture buffer should be released:
if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
v4l2_m2m_job_finish(...);
return;
}
And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
and, if set, skip returning the capture buffer to vb2.
This has only been compile tested, and this is missing the documentation. I would
like to know if this works in a real driver first before turning this into a proper
patch series.
Regards,
Hans
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index fb9ac7696fc6..83372de9b815 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -50,7 +50,8 @@ module_param(debug, int, 0644);
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 | \
- V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
+ V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
+ V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
/*
* __verify_planes_array() - verify that the planes array passed in struct
@@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
*/
vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
vbuf->field = b->field;
+ if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
+ vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
} else {
/* Zero any output buffer flags as this is a capture buffer */
vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
@@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
*caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
if (q->io_modes & VB2_DMABUF)
*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
+ if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
+ *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
#ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
if (q->supports_requests)
*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index bb3e63d6bd1a..6af1d519938f 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
struct vb2_v4l2_buffer *cap_vb,
bool copy_frame_flags);
+/**
+ * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
+ * released
+ *
+ * @out_vb: the output buffer
+ * @cap_vb: the capture buffer
+ *
+ * This helper function returns true if the current capture buffer should
+ * be released to vb2. This is the case if the output buffer specified that
+ * the capture buffer should be held (i.e. not returned to vb2) AND if the
+ * timestamp of the capture buffer differs from the output buffer timestamp.
+ *
+ * This helper is to be called at the start of the device_run callback:
+ *
+ * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
+ * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
+ * v4l2_m2m_job_finish(...);
+ * return;
+ * }
+ *
+ * This allows for multiple output buffers to be used to fill in a single
+ * capture buffer. This is typically used by stateless decoders where
+ * multiple e.g. H.264 slices contribute to a single decoded frame.
+ */
+static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
+ const struct vb2_v4l2_buffer *cap_vb)
+{
+ return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
+ cap_vb->vb2_buf.copied_timestamp &&
+ out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
+}
+
/* v4l2 request helper */
void v4l2_m2m_request_queue(struct media_request *req);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 22f3ff76a8b5..e9d99023c637 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -504,6 +504,8 @@ struct vb2_buf_ops {
* @buf_ops: callbacks to deliver buffer information.
* between user-space and kernel-space.
* @drv_priv: driver private data.
+ * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
+ * by the vb2 core.
* @buf_struct_size: size of the driver-specific buffer structure;
* "0" indicates the driver doesn't want to use a custom buffer
* structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
@@ -570,6 +572,7 @@ struct vb2_queue {
const struct vb2_buf_ops *buf_ops;
void *drv_priv;
+ u32 subsystem_flags;
unsigned int buf_struct_size;
u32 timestamp_flags;
gfp_t gfp_flags;
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 8a10889dc2fd..4e52427a4114 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
struct vb2_plane planes[VB2_MAX_PLANES];
};
+/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
+#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
+
/*
* to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
*/
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1050a75fb7ef..26925d63ea05 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
};
/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
-#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
-#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
-#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
-#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
-#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
+#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
+#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
+#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
+#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
+#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
+#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
/**
* struct v4l2_plane - plane info for multi-planar buffers
@@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
#define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
/* timecode field is valid */
#define V4L2_BUF_FLAG_TIMECODE 0x00000100
+/* Don't return the capture buffer until OUTPUT timestamp changes */
+#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
/* Buffer is prepared for queuing */
#define V4L2_BUF_FLAG_PREPARED 0x00000400
/* Cache handling flags */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
2019-04-30 8:31 [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF Hans Verkuil
@ 2019-05-13 9:36 ` Hans Verkuil
2019-05-13 17:19 ` Nicolas Dufresne
2019-05-14 10:01 ` Alexandre Courbot
0 siblings, 2 replies; 6+ messages in thread
From: Hans Verkuil @ 2019-05-13 9:36 UTC (permalink / raw)
To: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa,
Nicolas Dufresne, Alexandre Courbot, Boris Brezillon,
Maxime Ripard, Thierry Reding
On 4/30/19 10:31 AM, Hans Verkuil wrote:
> This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
> It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
> a v4l2_m2m_release_capture_buf() helper function.
>
> Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> to indicate support for this flag.
>
> At the start of the device_run function drivers should first check if the
> current capture buffer should be released:
>
> if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> v4l2_m2m_job_finish(...);
> return;
> }
>
> And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> and, if set, skip returning the capture buffer to vb2.
>
> This has only been compile tested, and this is missing the documentation. I would
> like to know if this works in a real driver first before turning this into a proper
> patch series.
Ping!
Did anyone have the opportunity (or is planning) to test if this patch actually works?
Regards,
Hans
>
> Regards,
>
> Hans
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index fb9ac7696fc6..83372de9b815 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -50,7 +50,8 @@ module_param(debug, int, 0644);
> 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 | \
> - V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
> + V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
> + V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>
> /*
> * __verify_planes_array() - verify that the planes array passed in struct
> @@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> */
> vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> vbuf->field = b->field;
> + if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> + vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
> } else {
> /* Zero any output buffer flags as this is a capture buffer */
> vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
> @@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
> if (q->io_modes & VB2_DMABUF)
> *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
> + if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
> + *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
> if (q->supports_requests)
> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index bb3e63d6bd1a..6af1d519938f 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
> struct vb2_v4l2_buffer *cap_vb,
> bool copy_frame_flags);
>
> +/**
> + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
> + * released
> + *
> + * @out_vb: the output buffer
> + * @cap_vb: the capture buffer
> + *
> + * This helper function returns true if the current capture buffer should
> + * be released to vb2. This is the case if the output buffer specified that
> + * the capture buffer should be held (i.e. not returned to vb2) AND if the
> + * timestamp of the capture buffer differs from the output buffer timestamp.
> + *
> + * This helper is to be called at the start of the device_run callback:
> + *
> + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> + * v4l2_m2m_job_finish(...);
> + * return;
> + * }
> + *
> + * This allows for multiple output buffers to be used to fill in a single
> + * capture buffer. This is typically used by stateless decoders where
> + * multiple e.g. H.264 slices contribute to a single decoded frame.
> + */
> +static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
> + const struct vb2_v4l2_buffer *cap_vb)
> +{
> + return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
> + cap_vb->vb2_buf.copied_timestamp &&
> + out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
> +}
> +
> /* v4l2 request helper */
>
> void v4l2_m2m_request_queue(struct media_request *req);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 22f3ff76a8b5..e9d99023c637 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -504,6 +504,8 @@ struct vb2_buf_ops {
> * @buf_ops: callbacks to deliver buffer information.
> * between user-space and kernel-space.
> * @drv_priv: driver private data.
> + * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
> + * by the vb2 core.
> * @buf_struct_size: size of the driver-specific buffer structure;
> * "0" indicates the driver doesn't want to use a custom buffer
> * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
> @@ -570,6 +572,7 @@ struct vb2_queue {
> const struct vb2_buf_ops *buf_ops;
>
> void *drv_priv;
> + u32 subsystem_flags;
> unsigned int buf_struct_size;
> u32 timestamp_flags;
> gfp_t gfp_flags;
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 8a10889dc2fd..4e52427a4114 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
> struct vb2_plane planes[VB2_MAX_PLANES];
> };
>
> +/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
> +#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
> +
> /*
> * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
> */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1050a75fb7ef..26925d63ea05 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
> };
>
> /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> -#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> -#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> -#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> -#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> -#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> +#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
>
> /**
> * struct v4l2_plane - plane info for multi-planar buffers
> @@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> #define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
> /* timecode field is valid */
> #define V4L2_BUF_FLAG_TIMECODE 0x00000100
> +/* Don't return the capture buffer until OUTPUT timestamp changes */
> +#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
> /* Buffer is prepared for queuing */
> #define V4L2_BUF_FLAG_PREPARED 0x00000400
> /* Cache handling flags */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
2019-05-13 9:36 ` Hans Verkuil
@ 2019-05-13 17:19 ` Nicolas Dufresne
2019-05-14 10:01 ` Alexandre Courbot
1 sibling, 0 replies; 6+ messages in thread
From: Nicolas Dufresne @ 2019-05-13 17:19 UTC (permalink / raw)
To: Hans Verkuil, Linux Media Mailing List, Paul Kocialkowski,
Tomasz Figa, Alexandre Courbot, Boris Brezillon, Maxime Ripard,
Thierry Reding
[-- Attachment #1: Type: text/plain, Size: 8206 bytes --]
Le lundi 13 mai 2019 à 11:36 +0200, Hans Verkuil a écrit :
> On 4/30/19 10:31 AM, Hans Verkuil wrote:
> > This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
> > It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
> > a v4l2_m2m_release_capture_buf() helper function.
> >
> > Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > to indicate support for this flag.
> >
> > At the start of the device_run function drivers should first check if the
> > current capture buffer should be released:
> >
> > if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> > v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> > v4l2_m2m_job_finish(...);
> > return;
> > }
> >
> > And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> > and, if set, skip returning the capture buffer to vb2.
> >
> > This has only been compile tested, and this is missing the documentation. I would
> > like to know if this works in a real driver first before turning this into a proper
> > patch series.
>
> Ping!
>
> Did anyone have the opportunity (or is planning) to test if this patch actually works?
On my side, Boris is still working on getting the very basic of the
driver working, but this is in our queue.
>
> Regards,
>
> Hans
>
> > Regards,
> >
> > Hans
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index fb9ac7696fc6..83372de9b815 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -50,7 +50,8 @@ module_param(debug, int, 0644);
> > 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 | \
> > - V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
> > + V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
> > + V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> >
> > /*
> > * __verify_planes_array() - verify that the planes array passed in struct
> > @@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> > */
> > vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> > vbuf->field = b->field;
> > + if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> > + vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
> > } else {
> > /* Zero any output buffer flags as this is a capture buffer */
> > vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
> > @@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
> > if (q->io_modes & VB2_DMABUF)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
> > + if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
> > + *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> > #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
> > if (q->supports_requests)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > index bb3e63d6bd1a..6af1d519938f 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
> > struct vb2_v4l2_buffer *cap_vb,
> > bool copy_frame_flags);
> >
> > +/**
> > + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
> > + * released
> > + *
> > + * @out_vb: the output buffer
> > + * @cap_vb: the capture buffer
> > + *
> > + * This helper function returns true if the current capture buffer should
> > + * be released to vb2. This is the case if the output buffer specified that
> > + * the capture buffer should be held (i.e. not returned to vb2) AND if the
> > + * timestamp of the capture buffer differs from the output buffer timestamp.
> > + *
> > + * This helper is to be called at the start of the device_run callback:
> > + *
> > + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> > + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> > + * v4l2_m2m_job_finish(...);
> > + * return;
> > + * }
> > + *
> > + * This allows for multiple output buffers to be used to fill in a single
> > + * capture buffer. This is typically used by stateless decoders where
> > + * multiple e.g. H.264 slices contribute to a single decoded frame.
> > + */
> > +static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
> > + const struct vb2_v4l2_buffer *cap_vb)
> > +{
> > + return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
> > + cap_vb->vb2_buf.copied_timestamp &&
> > + out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
> > +}
> > +
> > /* v4l2 request helper */
> >
> > void v4l2_m2m_request_queue(struct media_request *req);
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 22f3ff76a8b5..e9d99023c637 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -504,6 +504,8 @@ struct vb2_buf_ops {
> > * @buf_ops: callbacks to deliver buffer information.
> > * between user-space and kernel-space.
> > * @drv_priv: driver private data.
> > + * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
> > + * by the vb2 core.
> > * @buf_struct_size: size of the driver-specific buffer structure;
> > * "0" indicates the driver doesn't want to use a custom buffer
> > * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
> > @@ -570,6 +572,7 @@ struct vb2_queue {
> > const struct vb2_buf_ops *buf_ops;
> >
> > void *drv_priv;
> > + u32 subsystem_flags;
> > unsigned int buf_struct_size;
> > u32 timestamp_flags;
> > gfp_t gfp_flags;
> > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> > index 8a10889dc2fd..4e52427a4114 100644
> > --- a/include/media/videobuf2-v4l2.h
> > +++ b/include/media/videobuf2-v4l2.h
> > @@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
> > struct vb2_plane planes[VB2_MAX_PLANES];
> > };
> >
> > +/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
> > +#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
> > +
> > /*
> > * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
> > */
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 1050a75fb7ef..26925d63ea05 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
> > };
> >
> > /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> > -#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> > -#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> > -#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> > -#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> > -#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> > +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> > +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> > +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> > +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> > +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> > +#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
> >
> > /**
> > * struct v4l2_plane - plane info for multi-planar buffers
> > @@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> > #define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
> > /* timecode field is valid */
> > #define V4L2_BUF_FLAG_TIMECODE 0x00000100
> > +/* Don't return the capture buffer until OUTPUT timestamp changes */
> > +#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
> > /* Buffer is prepared for queuing */
> > #define V4L2_BUF_FLAG_PREPARED 0x00000400
> > /* Cache handling flags */
> >
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
2019-05-13 9:36 ` Hans Verkuil
2019-05-13 17:19 ` Nicolas Dufresne
@ 2019-05-14 10:01 ` Alexandre Courbot
2019-05-14 10:59 ` Hans Verkuil
1 sibling, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2019-05-14 10:01 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa,
Nicolas Dufresne, Boris Brezillon, Maxime Ripard, Thierry Reding
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Date: Mon, May 13, 2019 at 6:36 PM
To: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa, Nicolas
Dufresne, Alexandre Courbot, Boris Brezillon, Maxime Ripard, Thierry
Reding
> On 4/30/19 10:31 AM, Hans Verkuil wrote:
> > This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
> > It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
> > a v4l2_m2m_release_capture_buf() helper function.
> >
> > Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > to indicate support for this flag.
> >
> > At the start of the device_run function drivers should first check if the
> > current capture buffer should be released:
> >
> > if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> > v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> > v4l2_m2m_job_finish(...);
> > return;
> > }
> >
> > And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> > and, if set, skip returning the capture buffer to vb2.
> >
> > This has only been compile tested, and this is missing the documentation. I would
> > like to know if this works in a real driver first before turning this into a proper
> > patch series.
>
> Ping!
>
> Did anyone have the opportunity (or is planning) to test if this patch actually works?
Sorry, I am also still bringing a test driver up as well. But at first
sight it seems to cover what we discussed.
Just one note regarding the naming of v4l2_m2m_release_capture_buf().
>
> Regards,
>
> Hans
>
> >
> > Regards,
> >
> > Hans
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index fb9ac7696fc6..83372de9b815 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -50,7 +50,8 @@ module_param(debug, int, 0644);
> > 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 | \
> > - V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
> > + V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
> > + V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> >
> > /*
> > * __verify_planes_array() - verify that the planes array passed in struct
> > @@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> > */
> > vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> > vbuf->field = b->field;
> > + if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> > + vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
> > } else {
> > /* Zero any output buffer flags as this is a capture buffer */
> > vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
> > @@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
> > if (q->io_modes & VB2_DMABUF)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
> > + if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
> > + *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> > #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
> > if (q->supports_requests)
> > *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > index bb3e63d6bd1a..6af1d519938f 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
> > struct vb2_v4l2_buffer *cap_vb,
> > bool copy_frame_flags);
> >
> > +/**
> > + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
> > + * released
> > + *
> > + * @out_vb: the output buffer
> > + * @cap_vb: the capture buffer
> > + *
> > + * This helper function returns true if the current capture buffer should
> > + * be released to vb2. This is the case if the output buffer specified that
> > + * the capture buffer should be held (i.e. not returned to vb2) AND if the
> > + * timestamp of the capture buffer differs from the output buffer timestamp.
> > + *
> > + * This helper is to be called at the start of the device_run callback:
> > + *
> > + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> > + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> > + * v4l2_m2m_job_finish(...);
> > + * return;
> > + * }
The way this function is called makes it sound like it does release
the capture buffer by itself - but wouldn't the current behavior
suggest it should rather be named something like
v4l2_m2m_should_release_capture_buf()? Either that or we call
v4l2_m2m_buf_done() and v4l2_m2m_job_finish() from it.
> > + *
> > + * This allows for multiple output buffers to be used to fill in a single
> > + * capture buffer. This is typically used by stateless decoders where
> > + * multiple e.g. H.264 slices contribute to a single decoded frame.
> > + */
> > +static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
> > + const struct vb2_v4l2_buffer *cap_vb)
> > +{
> > + return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
> > + cap_vb->vb2_buf.copied_timestamp &&
> > + out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
> > +}
> > +
> > /* v4l2 request helper */
> >
> > void v4l2_m2m_request_queue(struct media_request *req);
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 22f3ff76a8b5..e9d99023c637 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -504,6 +504,8 @@ struct vb2_buf_ops {
> > * @buf_ops: callbacks to deliver buffer information.
> > * between user-space and kernel-space.
> > * @drv_priv: driver private data.
> > + * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
> > + * by the vb2 core.
> > * @buf_struct_size: size of the driver-specific buffer structure;
> > * "0" indicates the driver doesn't want to use a custom buffer
> > * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
> > @@ -570,6 +572,7 @@ struct vb2_queue {
> > const struct vb2_buf_ops *buf_ops;
> >
> > void *drv_priv;
> > + u32 subsystem_flags;
> > unsigned int buf_struct_size;
> > u32 timestamp_flags;
> > gfp_t gfp_flags;
> > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> > index 8a10889dc2fd..4e52427a4114 100644
> > --- a/include/media/videobuf2-v4l2.h
> > +++ b/include/media/videobuf2-v4l2.h
> > @@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
> > struct vb2_plane planes[VB2_MAX_PLANES];
> > };
> >
> > +/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
> > +#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
> > +
> > /*
> > * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
> > */
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 1050a75fb7ef..26925d63ea05 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
> > };
> >
> > /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> > -#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> > -#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> > -#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> > -#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> > -#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> > +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> > +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> > +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> > +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> > +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> > +#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
> >
> > /**
> > * struct v4l2_plane - plane info for multi-planar buffers
> > @@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> > #define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
> > /* timecode field is valid */
> > #define V4L2_BUF_FLAG_TIMECODE 0x00000100
> > +/* Don't return the capture buffer until OUTPUT timestamp changes */
> > +#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
> > /* Buffer is prepared for queuing */
> > #define V4L2_BUF_FLAG_PREPARED 0x00000400
> > /* Cache handling flags */
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
2019-05-14 10:01 ` Alexandre Courbot
@ 2019-05-14 10:59 ` Hans Verkuil
2019-05-16 8:44 ` Alexandre Courbot
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2019-05-14 10:59 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa,
Nicolas Dufresne, Boris Brezillon, Maxime Ripard, Thierry Reding
On 5/14/19 12:01 PM, Alexandre Courbot wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Date: Mon, May 13, 2019 at 6:36 PM
> To: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa, Nicolas
> Dufresne, Alexandre Courbot, Boris Brezillon, Maxime Ripard, Thierry
> Reding
>
>> On 4/30/19 10:31 AM, Hans Verkuil wrote:
>>> This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
>>> It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
>>> a v4l2_m2m_release_capture_buf() helper function.
>>>
>>> Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
>>> to indicate support for this flag.
>>>
>>> At the start of the device_run function drivers should first check if the
>>> current capture buffer should be released:
>>>
>>> if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
>>> v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
>>> v4l2_m2m_job_finish(...);
>>> return;
>>> }
>>>
>>> And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>>> and, if set, skip returning the capture buffer to vb2.
>>>
>>> This has only been compile tested, and this is missing the documentation. I would
>>> like to know if this works in a real driver first before turning this into a proper
>>> patch series.
>>
>> Ping!
>>
>> Did anyone have the opportunity (or is planning) to test if this patch actually works?
>
> Sorry, I am also still bringing a test driver up as well. But at first
> sight it seems to cover what we discussed.
OK, I'll wait for test results. I just wanted to make sure this didn't snow under.
>
> Just one note regarding the naming of v4l2_m2m_release_capture_buf().
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index fb9ac7696fc6..83372de9b815 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -50,7 +50,8 @@ module_param(debug, int, 0644);
>>> 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 | \
>>> - V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
>>> + V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
>>> + V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>>>
>>> /*
>>> * __verify_planes_array() - verify that the planes array passed in struct
>>> @@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>>> */
>>> vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
>>> vbuf->field = b->field;
>>> + if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
>>> + vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
>>> } else {
>>> /* Zero any output buffer flags as this is a capture buffer */
>>> vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
>>> @@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
>>> if (q->io_modes & VB2_DMABUF)
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
>>> + if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
>>> + *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
>>> #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
>>> if (q->supports_requests)
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>> index bb3e63d6bd1a..6af1d519938f 100644
>>> --- a/include/media/v4l2-mem2mem.h
>>> +++ b/include/media/v4l2-mem2mem.h
>>> @@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
>>> struct vb2_v4l2_buffer *cap_vb,
>>> bool copy_frame_flags);
>>>
>>> +/**
>>> + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
>>> + * released
>>> + *
>>> + * @out_vb: the output buffer
>>> + * @cap_vb: the capture buffer
>>> + *
>>> + * This helper function returns true if the current capture buffer should
>>> + * be released to vb2. This is the case if the output buffer specified that
>>> + * the capture buffer should be held (i.e. not returned to vb2) AND if the
>>> + * timestamp of the capture buffer differs from the output buffer timestamp.
>>> + *
>>> + * This helper is to be called at the start of the device_run callback:
>>> + *
>>> + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
>>> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
>>> + * v4l2_m2m_job_finish(...);
>>> + * return;
>>> + * }
>
> The way this function is called makes it sound like it does release
> the capture buffer by itself - but wouldn't the current behavior
> suggest it should rather be named something like
> v4l2_m2m_should_release_capture_buf()? Either that or we call
> v4l2_m2m_buf_done() and v4l2_m2m_job_finish() from it.
I agree, this can be named better. How about v4l2_m2m_can_release_capture_buffer()?
Regards,
Hans
>
>>> + *
>>> + * This allows for multiple output buffers to be used to fill in a single
>>> + * capture buffer. This is typically used by stateless decoders where
>>> + * multiple e.g. H.264 slices contribute to a single decoded frame.
>>> + */
>>> +static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
>>> + const struct vb2_v4l2_buffer *cap_vb)
>>> +{
>>> + return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
>>> + cap_vb->vb2_buf.copied_timestamp &&
>>> + out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
>>> +}
>>> +
>>> /* v4l2 request helper */
>>>
>>> void v4l2_m2m_request_queue(struct media_request *req);
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index 22f3ff76a8b5..e9d99023c637 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -504,6 +504,8 @@ struct vb2_buf_ops {
>>> * @buf_ops: callbacks to deliver buffer information.
>>> * between user-space and kernel-space.
>>> * @drv_priv: driver private data.
>>> + * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
>>> + * by the vb2 core.
>>> * @buf_struct_size: size of the driver-specific buffer structure;
>>> * "0" indicates the driver doesn't want to use a custom buffer
>>> * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
>>> @@ -570,6 +572,7 @@ struct vb2_queue {
>>> const struct vb2_buf_ops *buf_ops;
>>>
>>> void *drv_priv;
>>> + u32 subsystem_flags;
>>> unsigned int buf_struct_size;
>>> u32 timestamp_flags;
>>> gfp_t gfp_flags;
>>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>>> index 8a10889dc2fd..4e52427a4114 100644
>>> --- a/include/media/videobuf2-v4l2.h
>>> +++ b/include/media/videobuf2-v4l2.h
>>> @@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
>>> struct vb2_plane planes[VB2_MAX_PLANES];
>>> };
>>>
>>> +/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
>>> +#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
>>> +
>>> /*
>>> * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
>>> */
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 1050a75fb7ef..26925d63ea05 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
>>> };
>>>
>>> /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
>>> -#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
>>> -#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
>>> -#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
>>> -#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
>>> -#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
>>> +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
>>> +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
>>> +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
>>> +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
>>> +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
>>> +#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
>>>
>>> /**
>>> * struct v4l2_plane - plane info for multi-planar buffers
>>> @@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
>>> #define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
>>> /* timecode field is valid */
>>> #define V4L2_BUF_FLAG_TIMECODE 0x00000100
>>> +/* Don't return the capture buffer until OUTPUT timestamp changes */
>>> +#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
>>> /* Buffer is prepared for queuing */
>>> #define V4L2_BUF_FLAG_PREPARED 0x00000400
>>> /* Cache handling flags */
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
2019-05-14 10:59 ` Hans Verkuil
@ 2019-05-16 8:44 ` Alexandre Courbot
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Courbot @ 2019-05-16 8:44 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa,
Nicolas Dufresne, Boris Brezillon, Maxime Ripard, Thierry Reding
On Tue, May 14, 2019 at 7:59 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 5/14/19 12:01 PM, Alexandre Courbot wrote:
> > From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Date: Mon, May 13, 2019 at 6:36 PM
> > To: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa, Nicolas
> > Dufresne, Alexandre Courbot, Boris Brezillon, Maxime Ripard, Thierry
> > Reding
> >
> >> On 4/30/19 10:31 AM, Hans Verkuil wrote:
> >>> This RFC patch adds support for the V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag.
> >>> It also adds a new V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF capability and
> >>> a v4l2_m2m_release_capture_buf() helper function.
> >>>
> >>> Drivers should set vb2_queue->subsystem_flags to VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> >>> to indicate support for this flag.
> >>>
> >>> At the start of the device_run function drivers should first check if the
> >>> current capture buffer should be released:
> >>>
> >>> if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> >>> v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> >>> v4l2_m2m_job_finish(...);
> >>> return;
> >>> }
> >>>
> >>> And at the end they should check (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> >>> and, if set, skip returning the capture buffer to vb2.
> >>>
> >>> This has only been compile tested, and this is missing the documentation. I would
> >>> like to know if this works in a real driver first before turning this into a proper
> >>> patch series.
> >>
> >> Ping!
> >>
> >> Did anyone have the opportunity (or is planning) to test if this patch actually works?
> >
> > Sorry, I am also still bringing a test driver up as well. But at first
> > sight it seems to cover what we discussed.
>
> OK, I'll wait for test results. I just wanted to make sure this didn't snow under.
>
> >
> > Just one note regarding the naming of v4l2_m2m_release_capture_buf().
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> ---
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> index fb9ac7696fc6..83372de9b815 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> @@ -50,7 +50,8 @@ module_param(debug, int, 0644);
> >>> 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 | \
> >>> - V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
> >>> + V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE | \
> >>> + V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> >>>
> >>> /*
> >>> * __verify_planes_array() - verify that the planes array passed in struct
> >>> @@ -321,6 +322,8 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> >>> */
> >>> vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> >>> vbuf->field = b->field;
> >>> + if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> >>> + vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
> >>> } else {
> >>> /* Zero any output buffer flags as this is a capture buffer */
> >>> vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
> >>> @@ -659,6 +662,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> >>> *caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
> >>> if (q->io_modes & VB2_DMABUF)
> >>> *caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
> >>> + if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
> >>> + *caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> >>> #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
> >>> if (q->supports_requests)
> >>> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> >>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> >>> index bb3e63d6bd1a..6af1d519938f 100644
> >>> --- a/include/media/v4l2-mem2mem.h
> >>> +++ b/include/media/v4l2-mem2mem.h
> >>> @@ -648,6 +648,38 @@ void v4l2_m2m_buf_copy_metadata(const struct vb2_v4l2_buffer *out_vb,
> >>> struct vb2_v4l2_buffer *cap_vb,
> >>> bool copy_frame_flags);
> >>>
> >>> +/**
> >>> + * v4l2_m2m_release_capture_buf() - check if the capture buffer should be
> >>> + * released
> >>> + *
> >>> + * @out_vb: the output buffer
> >>> + * @cap_vb: the capture buffer
> >>> + *
> >>> + * This helper function returns true if the current capture buffer should
> >>> + * be released to vb2. This is the case if the output buffer specified that
> >>> + * the capture buffer should be held (i.e. not returned to vb2) AND if the
> >>> + * timestamp of the capture buffer differs from the output buffer timestamp.
> >>> + *
> >>> + * This helper is to be called at the start of the device_run callback:
> >>> + *
> >>> + * if (v4l2_m2m_release_capture_buf(out_vb, cap_vb)) {
> >>> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE);
> >>> + * v4l2_m2m_job_finish(...);
> >>> + * return;
> >>> + * }
> >
> > The way this function is called makes it sound like it does release
> > the capture buffer by itself - but wouldn't the current behavior
> > suggest it should rather be named something like
> > v4l2_m2m_should_release_capture_buf()? Either that or we call
> > v4l2_m2m_buf_done() and v4l2_m2m_job_finish() from it.
>
> I agree, this can be named better. How about v4l2_m2m_can_release_capture_buffer()?
Sounds good to me!
>
> Regards,
>
> Hans
>
> >
> >>> + *
> >>> + * This allows for multiple output buffers to be used to fill in a single
> >>> + * capture buffer. This is typically used by stateless decoders where
> >>> + * multiple e.g. H.264 slices contribute to a single decoded frame.
> >>> + */
> >>> +static inline bool v4l2_m2m_release_capture_buf(const struct vb2_v4l2_buffer *out_vb,
> >>> + const struct vb2_v4l2_buffer *cap_vb)
> >>> +{
> >>> + return (out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF) &&
> >>> + cap_vb->vb2_buf.copied_timestamp &&
> >>> + out_vb->vb2_buf.timestamp != cap_vb->vb2_buf.timestamp;
> >>> +}
> >>> +
> >>> /* v4l2 request helper */
> >>>
> >>> void v4l2_m2m_request_queue(struct media_request *req);
> >>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> >>> index 22f3ff76a8b5..e9d99023c637 100644
> >>> --- a/include/media/videobuf2-core.h
> >>> +++ b/include/media/videobuf2-core.h
> >>> @@ -504,6 +504,8 @@ struct vb2_buf_ops {
> >>> * @buf_ops: callbacks to deliver buffer information.
> >>> * between user-space and kernel-space.
> >>> * @drv_priv: driver private data.
> >>> + * @subsystem_flags: Flags specific to the subsystem (V4L2/DVB/etc.). Not used
> >>> + * by the vb2 core.
> >>> * @buf_struct_size: size of the driver-specific buffer structure;
> >>> * "0" indicates the driver doesn't want to use a custom buffer
> >>> * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
> >>> @@ -570,6 +572,7 @@ struct vb2_queue {
> >>> const struct vb2_buf_ops *buf_ops;
> >>>
> >>> void *drv_priv;
> >>> + u32 subsystem_flags;
> >>> unsigned int buf_struct_size;
> >>> u32 timestamp_flags;
> >>> gfp_t gfp_flags;
> >>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> >>> index 8a10889dc2fd..4e52427a4114 100644
> >>> --- a/include/media/videobuf2-v4l2.h
> >>> +++ b/include/media/videobuf2-v4l2.h
> >>> @@ -49,6 +49,9 @@ struct vb2_v4l2_buffer {
> >>> struct vb2_plane planes[VB2_MAX_PLANES];
> >>> };
> >>>
> >>> +/* VB2 V4L2 flags as set in vb2_queue.subsystem_flags */
> >>> +#define VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 0)
> >>> +
> >>> /*
> >>> * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer *
> >>> */
> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>> index 1050a75fb7ef..26925d63ea05 100644
> >>> --- a/include/uapi/linux/videodev2.h
> >>> +++ b/include/uapi/linux/videodev2.h
> >>> @@ -907,11 +907,12 @@ struct v4l2_requestbuffers {
> >>> };
> >>>
> >>> /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> >>> -#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> >>> -#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> >>> -#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> >>> -#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> >>> -#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> >>> +#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
> >>>
> >>> /**
> >>> * struct v4l2_plane - plane info for multi-planar buffers
> >>> @@ -1033,6 +1034,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> >>> #define V4L2_BUF_FLAG_IN_REQUEST 0x00000080
> >>> /* timecode field is valid */
> >>> #define V4L2_BUF_FLAG_TIMECODE 0x00000100
> >>> +/* Don't return the capture buffer until OUTPUT timestamp changes */
> >>> +#define V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF 0x00000200
> >>> /* Buffer is prepared for queuing */
> >>> #define V4L2_BUF_FLAG_PREPARED 0x00000400
> >>> /* Cache handling flags */
> >>>
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-16 8:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-30 8:31 [RFC PATCH] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF Hans Verkuil
2019-05-13 9:36 ` Hans Verkuil
2019-05-13 17:19 ` Nicolas Dufresne
2019-05-14 10:01 ` Alexandre Courbot
2019-05-14 10:59 ` Hans Verkuil
2019-05-16 8:44 ` Alexandre Courbot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.