On 5/14/19 12:01 PM, Alexandre Courbot wrote:
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> 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@xxxxxxxxx>
>>> ---
>>> 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 */
>>>
>>
