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=-5.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 70508C433E0 for ; Mon, 21 Dec 2020 05:03:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1C42A22C7B for ; Mon, 21 Dec 2020 05:03:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726704AbgLUFD6 (ORCPT ); Mon, 21 Dec 2020 00:03:58 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40110 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725984AbgLUFD6 (ORCPT ); Mon, 21 Dec 2020 00:03:58 -0500 Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 77B28C061282 for ; Sun, 20 Dec 2020 21:03:17 -0800 (PST) Received: by mail-ej1-x631.google.com with SMTP id lt17so11683949ejb.3 for ; Sun, 20 Dec 2020 21:03:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JSDBpbAb+/VW+RgOV1OSYF43CooUkraHhPfK0IhMgi4=; b=cf8HHgfeCrEt06Dq61ckFkc/qILM4IrFeIP8bQNnqWyTh/0UhG4cI1FIZ8YaJugCBH 0g+Jvedgj1D50OK5h40yKwQsYcV/B5piZQqbmX1DDcwt9mKCOBD26gdGHc3O1SuRfJmL 8EYfqbiCUWj/AnIXhmdjmXMKLltgk0mro30O4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JSDBpbAb+/VW+RgOV1OSYF43CooUkraHhPfK0IhMgi4=; b=qA1evGYDF7P7dJ+NFzQwCYPNE4L11t0Z3JgKtCSUwfHCh3LmuB/1GE0U018bBpKx2G wfaa9WgXZP7rgXIOaB4OEF7bO2URgS1Krq0r+Uij5cSjQN/JxjOIkgfnKni92Rho88RG nKriIiu36GM2hWVo4EJhRbi6lTfTbdOuy7A1CPDeY/hf4gKxVc/FivPubAjyS39VXVgV Qby9L1A1FKzemPWP191/C+9Ltnk9/OuRm9D/FMed/9ZPKnM2fx5OptKtTJE8Elw3fsgH gJAP07zOO2lwh1XDDLhaHLLrJ+fDiMVPgOv0gmYhAOFcmgKkydTsQi2PZP37RAX7zpQO MEfQ== X-Gm-Message-State: AOAM531wQlY+wAxNNShb+roYLD6nht96hTOFncYbYeEI4mbMsiwyXh+C 0Zso5x33KLqowKOOPelMHr8F2QsHGQoVxQ== X-Google-Smtp-Source: ABdhPJyPc9ZhQYgr/F+mkUPcYGulFPilVcttalmrzP4mi9zmJzm8z97v1v70k+Ki8shCS30YI0rA/Q== X-Received: by 2002:a17:906:5f97:: with SMTP id a23mr13972867eju.128.1608520398009; Sun, 20 Dec 2020 19:13:18 -0800 (PST) Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com. [209.85.128.50]) by smtp.gmail.com with ESMTPSA id dg10sm6209951edb.63.2020.12.20.19.13.16 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 20 Dec 2020 19:13:16 -0800 (PST) Received: by mail-wm1-f50.google.com with SMTP id e25so9558845wme.0 for ; Sun, 20 Dec 2020 19:13:16 -0800 (PST) X-Received: by 2002:a1c:c308:: with SMTP id t8mr14451460wmf.22.1608520395846; Sun, 20 Dec 2020 19:13:15 -0800 (PST) MIME-Version: 1.0 References: <20200804192939.2251988-1-helen.koike@collabora.com> <20200804192939.2251988-3-helen.koike@collabora.com> <3ac23162-ce59-6cc3-da48-90f26c618345@collabora.com> <4fec6e91-a19b-b0be-d4b6-72a333451d9b@collabora.com> In-Reply-To: <4fec6e91-a19b-b0be-d4b6-72a333451d9b@collabora.com> From: Tomasz Figa Date: Mon, 21 Dec 2020 12:13:04 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 2/7] media: v4l2: Add extended buffer operations To: Helen Koike Cc: Hans Verkuil , Mauro Carvalho Chehab , Hans Verkuil , Laurent Pinchart , Sakari Ailus , Linux Media Mailing List , Boris Brezillon , Hirokazu Honda , Nicolas Dufresne , Brian Starkey , kernel@collabora.com, Neil Armstrong , Linux Kernel Mailing List , Fritz Koenig , Maxime Jourdan , Stanimir Varbanov Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 17, 2020 at 10:20 PM Helen Koike wrote: > > Hi Tomasz, > > Thanks for your comments, I have a few questions below. > > On 12/16/20 12:13 AM, Tomasz Figa wrote: > > On Tue, Dec 15, 2020 at 11:37 PM Helen Koike wrote: > >> > >> Hi Tomasz, > >> > >> On 12/14/20 7:46 AM, Tomasz Figa wrote: > >>> On Fri, Dec 4, 2020 at 4:52 AM Helen Koike wrote: > >>>> > >>>> Hi, > >>>> > >>>> Please see my 2 points below (about v4l2_ext_buffer and another about timestamp). > >>>> > >>>> On 12/3/20 12:11 PM, Hans Verkuil wrote: > >>>>> On 23/11/2020 18:40, Helen Koike wrote: > >>>>>> > >>>>>> > >>>>>> On 11/23/20 12:46 PM, Tomasz Figa wrote: > >>>>>>> On Tue, Nov 24, 2020 at 12:08 AM Helen Koike wrote: > >>>>>>>> > >>>>>>>> Hi Hans, > >>>>>>>> > >>>>>>>> Thank you for your review. > >>>>>>>> > >>>>>>>> On 9/9/20 9:27 AM, Hans Verkuil wrote: > >>>>>>>>> Hi Helen, > >>>>>>>>> > >>>>>>>>> Again I'm just reviewing the uAPI. > >>>>>>>>> > >>>>>>>>> On 04/08/2020 21:29, Helen Koike wrote: [snip] > > > >> > >> Output: userspace fills plane information, informing in which memory buffer each > >> plane was placed (Or should this be pre-determined by the driver?) > >> > >> For MMAP > >> ----------------------- > >> userspace performs EXT_CREATE_BUF ioctl to reserve a buffer "index" range in > >> that mode, to be used in EXT_QBUF and EXT_DQBUF > >> > >> Should the API allow userspace to select how many memory buffers it wants? > >> (maybe not) > > > > I think it does allow that - it accepts the v4l2_ext_format struct. > > hmmm, I thought v4l2_ext_format would describe color planes, and not memory planes. > Should it describe memory planes instead? Since planes are defined by the pixelformat. > But is this information relevant to ext_{set/get/try} format? > Good point. I ended up assuming the current convention, where giving an M format would imply num_memory_planes == num_color_planes and non-M format num_memory_planes == 1. Sounds like we might want something like a flags field and that could have bits defined to select that. I think it would actually be useful for S_FMT as well, because that's what REQBUFS would use. > > > >> > >> userspace performs EXT_QUERY_MMAP_BUF to get the mmap offset/cookie and length > >> for each memory buffer. > >> > >> On EXT_QBUF, userspace doesn't need to fill membuf information. Should the > >> mmap offset and length be filled by the kernel and returned to userspace here > >> as well? I'm leaning towards: no. > > > > Yeah, based on my comment above, I think the answer should be no. > > > >> > >> If the answer is no, then here is my proposal: > >> ---------------------------------------------- > >> > >> /* If MMAP, drivers decide how many memory buffers to allocate */ > >> int ioctl( int fd, VIDIOC_EXT_CREATE_BUFS, struct v4l2_ext_buffer *argp ) > >> > >> /* Returns -EINVAL if not MMAP */ > >> int ioctl( int fd, VIDIOC_EXT_MMAP_QUERYBUF, struct v4l2_ext_mmap_querybuf *argp ) > >> > >> /* userspace fills v4l2_ext_buffer.membufs if DMA-fd or Userptr, leave it zero for MMAP > >> * Should userspace also fill v4l2_ext_buffer.planes? > >> */ > >> int ioctl( int fd, VIDIOC_EXT_QBUF, struct v4l2_ext_buffer *argp ) > >> > >> /* v4l2_ext_buffer.membufs is set to zero by the driver */ > >> int ioctl( int fd, VIDIOC_EXT_DBUF, struct v4l2_ext_buffer *argp ) > >> > >> (I omitted reserved fields below) > >> > >> struct v4l2_ext_create_buffers { > >> __u32 index; > >> __u32 count; > >> __u32 memory; > >> __u32 capabilities; > >> struct v4l2_ext_pix_format format; > >> }; > >> > >> struct v4l2_ext_mmap_membuf { > >> __u32 offset; > >> __u32 length; > >> } > >> > >> struct v4l2_ext_mmap_querybuf { > >> __u32 index; > >> struct v4l2_ext_mmap_membuf membufs[VIDEO_MAX_PLANES]; > >> } > >> > >> struct v4l2_ext_membuf { > >> __u32 memory; > >> union { > >> __u64 userptr; > >> __s32 dmabuf_fd; > >> } m; > >> // Can't we just remove the union and "memory" field, and the non-zero > >> // is the one we should use? > > > > I think that would lead to an equivalent result in this case. That > > said, I'm not sure if there would be any significant enough benefit to > > justify moving away from the current convention. Having the memory > > field might also make the structure a bit less error prone, e.g. > > resilient to missing memset(). > > > >> }; > >> > >> struct v4l2_ext_plane { > >> __u32 membuf_index; > >> __u32 offset; > >> __u32 bytesused; > >> }; > >> > >> struct v4l2_ext_buffer { > >> __u32 index; > >> __u32 type; > >> __u32 field; > >> __u32 sequence; > >> __u64 flags; > >> __u64 timestamp; > >> struct v4l2_ext_membuf membufs[VIDEO_MAX_PLANES]; > >> struct v4l2_ext_plane planes[VIDEO_MAX_PLANES]; > > > > Do we actually need this split into membufs and planes here? After > > all, all we want to pass to the kernel here is in what buffer the > > plane is in. > > You are right, we don't. > > > > > struct v4l2_ext_plane { > > __u32 memory; > > Should we design the API to allow a buffer to contain multiple memory planes > of different types? Lets say one memplane is DMA-fd, the other is userptr. > If the answer is yes, then struct v4l2_ext_create_buffers requires some changes. > If not, then there is no need a "memory" field per memory plane in a buffer. > That's a good question. I haven't seen any practical need to do that. Moreover, I suspect that the API might be going towards the DMA-buf centric model, with DMA-buf heaps getting upstream acceptance, so maybe we would be fine moving the memory field to the buffer struct indeed. > > union { > > __u32 membuf_index; > > __u64 userptr; > > __s32 dmabuf_fd; > > } m; > > __u32 offset; > > __u32 bytesused; > > We also need userptr_length right? Is it actually needed? The length of the plane is determined by the current format. I can only see as it being an extra sanity check before accessing the process memory, but is it necessary? I think I want to hear others's opinion on this. [snip] Best regards, Tomasz