From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8D386C31E40 for ; Thu, 15 Aug 2019 11:53:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 574D3206C2 for ; Thu, 15 Aug 2019 11:53:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730558AbfHOLxK (ORCPT ); Thu, 15 Aug 2019 07:53:10 -0400 Received: from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:37251 "EHLO lb2-smtp-cloud8.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730363AbfHOLxK (ORCPT ); Thu, 15 Aug 2019 07:53:10 -0400 Received: from [IPv6:2001:420:44c1:2579:f038:4b04:f67a:276f] ([IPv6:2001:420:44c1:2579:f038:4b04:f67a:276f]) by smtp-cloud8.xs4all.net with ESMTPA id yEJThvN7zDqPeyEJWhXwGV; Thu, 15 Aug 2019 13:53:06 +0200 Subject: Re: [PATCHv2 09/12] vb2: add V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF To: Alexandre Courbot Cc: Linux Media Mailing List , Tomasz Figa , Boris Brezillon References: <20190812110513.23774-1-hverkuil-cisco@xs4all.nl> <20190812110513.23774-10-hverkuil-cisco@xs4all.nl> From: Hans Verkuil Message-ID: <4961c8f5-0cb8-1ab9-af13-9f8092536e21@xs4all.nl> Date: Thu, 15 Aug 2019 13:53:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfPYPmtm9HzeIlB7+sKBux0u9yqtEkWNYacvU2aJ08Z+Qpv6e0C70d31SDp2MskdCEPr7gabt1qivWBcRvU1axzCPwD98CglFWuzcENSiFYTBazhPg3dE 7E4+YZVNE3WXZwmENE3eYZE5BqU3Sis77M6ycdJPeX1bpKKyqZXAWMIkXIRZuYHqEGLNbREAlFf2Kx2BDiDuAxSdvm1lGasskWcc9R2EF44Yf0gxe7ZxhYzp Q02u1sFqniYIeZupKekxYOfvJRU8Z+3ZBr1a5qYPio+PNk9R3880wk0tGqwfRr/hyfAzcpyPI7PHcfncK5z93w5x8vgaSUvIdNcstkr6l/YlOtMls9KAdKpP ZCnPPu5nOMa8ZIItoW+W6VBH3OfNkUFZeP/+AGllADQR1iVqp44= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On 8/15/19 10:12 AM, Alexandre Courbot wrote: > On Mon, Aug 12, 2019 at 8:06 PM 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. >> >> The device_run() function should look like this: >> >> 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; >> } >> cap_vb->is_held = out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> >> ... >> >> v4l2_m2m_buf_done(out_vb, VB2_BUF_STATE_DONE); >> if (!cap_vb->is_held) { >> v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> v4l2_m2m_job_finish(...); >> } >> >> In order to handle the corner case where V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF >> is always set for the output buffer, and you reached the last frame (so no >> new output buffer will be queued with a new timestamp), the driver should >> implement support for the V4L2_DEC_CMD_STOP command, and that should do: V4L2_DEC_CMD_STOP should have been CMD_FLUSH. Sorry for the confusion. > > I wonder if "the driver must implement..." would not make things > easier: what about drivers that only support formats for which we have > an exact 1:1 mapping (like VP8)? Should they also implement it? What Such drivers do not need this and will not set V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF. If that cap is not set, then V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF will be ignored. > it permitted for a driver supporting this flag to not implement > V4L2_DEC_CMD_STOP? Drivers that support this flag must implement DEC_CMD_FLUSH. > > Leaving this as an option may complicate user-space quite a bit. Since > the implementation of V4L2_DEC_CMD_STOP does not seem too complicated, > how about making it mandatory for all stateless decoders? It's a good question: should we support DEC_CMD_FLUSH also for decoders that do not set V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF? It would be a NOP, but it might simplify applications. That said, I think we can postpone a decision about that until we have more experience. It is easier to add support for this than to remove it later. Regards, Hans > > >> >> struct vb2_v4l2_buffer *out_vb = v4l2_m2m_last_src_buf(m2m_ctx); >> struct vb2_v4l2_buffer *cap_vb = v4l2_m2m_last_dst_buf(m2m_ctx); >> >> if (out_vb) { >> out_vb->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> } else if (cap_vb && cap_vb->is_held) { >> v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> v4l2_m2m_job_finish(...); >> } >> >> At least, I think so. Comments on this are very welcome. We definitely >> need better support in v4l2-mem2mem.c for such situations (same for >> stateful codecs) since it's too complex for drivers to get right IMHO. >> >> Regards, >> >> Hans >> >> Signed-off-by: Hans Verkuil >> Tested-by: Boris Brezillon >> Reviewed-by: Boris Brezillon >> --- >> Documentation/media/uapi/v4l/buffer.rst | 13 ++++++ >> .../media/uapi/v4l/vidioc-reqbufs.rst | 6 +++ >> .../media/common/videobuf2/videobuf2-v4l2.c | 8 +++- >> include/media/v4l2-mem2mem.h | 42 +++++++++++++++++++ >> include/media/videobuf2-core.h | 3 ++ >> include/media/videobuf2-v4l2.h | 5 +++ >> include/uapi/linux/videodev2.h | 13 +++--- >> 7 files changed, 84 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst >> index 1cbd9cde57f3..afb03906ead9 100644 >> --- a/Documentation/media/uapi/v4l/buffer.rst >> +++ b/Documentation/media/uapi/v4l/buffer.rst >> @@ -607,6 +607,19 @@ Buffer Flags >> applications shall use this flag for output buffers if the data in >> this buffer has not been created by the CPU but by some >> DMA-capable unit, in which case caches have not been used. >> + * .. _`V4L2-BUF-FLAG-M2M-HOLD-CAPTURE-BUF`: >> + >> + - ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` >> + - 0x00000200 >> + - Only valid if ``V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF`` is >> + set. It is typically used with stateless decoders where multiple >> + output buffers each decode to a slice of the decoded frame. >> + Applications can set this flag when queueing the output buffer >> + to prevent the driver from dequeueing the capture buffer after >> + the output buffer has been decoded (i.e. the capture buffer is >> + 'held'). If the timestamp of this output buffer differs from that >> + of the previous output buffer, then that indicates the start of a >> + new frame and the previously held capture buffer is dequeued. >> * .. _`V4L2-BUF-FLAG-LAST`: >> >> - ``V4L2_BUF_FLAG_LAST`` >> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> index d7faef10e39b..d0c643db477a 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst >> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit >> .. _V4L2-BUF-CAP-SUPPORTS-DMABUF: >> .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS: >> .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS: >> +.. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF: >> >> .. cssclass:: longtable >> >> @@ -150,6 +151,11 @@ aborting or finishing any DMA in progress, an implicit >> - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still >> mapped or exported via DMABUF. These orphaned buffers will be freed >> when they are unmapped or when the exported DMABUF fds are closed. >> + * - ``V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF`` >> + - 0x00000020 >> + - Only valid for stateless decoders. If set, then userspace can set the >> + ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag to hold off on returning the >> + capture buffer until the OUTPUT timestamp changes. >> >> Return Value >> ============ >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index 5a9ba3846f0a..699787f48f46 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 >> @@ -194,6 +195,7 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b >> } >> vbuf->sequence = 0; >> vbuf->request_fd = -1; >> + vbuf->is_held = false; >> >> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { >> switch (b->memory) { >> @@ -321,6 +323,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; >> @@ -654,6 +658,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 0b9c3a287061..724858efded2 100644 >> --- a/include/media/v4l2-mem2mem.h >> +++ b/include/media/v4l2-mem2mem.h >> @@ -644,6 +644,48 @@ 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: >> + * >> + * .. code-block:: c >> + * >> + * 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; >> + * } >> + * cap_vb->is_held = out_vb->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF; >> + * >> + * ... >> + * >> + * v4l2_m2m_buf_done(out_vb, VB2_BUF_STATE_DONE); >> + * if (!cap_vb->is_held) { >> + * v4l2_m2m_buf_done(cap_vb, VB2_BUF_STATE_DONE); >> + * v4l2_m2m_job_finish(...); >> + * } >> + * >> + * 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 cap_vb->is_held && 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 640aabe69450..a2b2208b02da 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -505,6 +505,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)`` >> @@ -571,6 +573,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..13ab101864aa 100644 >> --- a/include/media/videobuf2-v4l2.h >> +++ b/include/media/videobuf2-v4l2.h >> @@ -33,6 +33,7 @@ >> * @timecode: frame timecode. >> * @sequence: sequence count of this frame. >> * @request_fd: the request_fd associated with this buffer >> + * @is_held: if true, then this buffer was held >> * @planes: plane information (userptr/fd, length, bytesused, data_offset). >> * >> * Should contain enough information to be able to cover all the fields >> @@ -46,9 +47,13 @@ struct vb2_v4l2_buffer { >> struct v4l2_timecode timecode; >> __u32 sequence; >> __s32 request_fd; >> + bool is_held; >> 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 530638dffd93..4fa9f543742d 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -915,11 +915,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 >> @@ -1041,6 +1042,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 */ >> -- >> 2.20.1 >>