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=-6.0 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 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 A249FC433DB for ; Fri, 8 Jan 2021 10:01:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 41833236F9 for ; Fri, 8 Jan 2021 10:01:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728195AbhAHKBQ (ORCPT ); Fri, 8 Jan 2021 05:01:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41426 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727569AbhAHKBP (ORCPT ); Fri, 8 Jan 2021 05:01:15 -0500 Received: from mail-ej1-x636.google.com (mail-ej1-x636.google.com [IPv6:2a00:1450:4864:20::636]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 73627C0612F4 for ; Fri, 8 Jan 2021 02:00:35 -0800 (PST) Received: by mail-ej1-x636.google.com with SMTP id 6so13768786ejz.5 for ; Fri, 08 Jan 2021 02:00:35 -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=RIPDrybHYDhrvRvtrv7fNkrHahASlP6RR/P3/LW+RDQ=; b=VLn2tfq+cDp5QXVPJu7O9dnF40qGvvxBpHn0n6yOHOwnNP+XP8cT6kPTUe1T23K+is OKV3bhfEzVswtI+ATO28Ul3IpAdT9JUzOSA+9XD9Q9oDn0GYgSAHWF4XuYp548lt6on4 J3QLISt2daUJJhWALz5NzsT3tqkTLp0OKVG18= 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=RIPDrybHYDhrvRvtrv7fNkrHahASlP6RR/P3/LW+RDQ=; b=CJfTPqchpshjfJzqbeYORZFUw/qsp7LHnn4tx/iXW8/PJhIjVzz4oUBet/wzcjuY4u 7XcmjkkbZh/XtM3qCOFnRHm/1O+ybEbg3XWq2iLo/5Y/HtUYw47VQ3fD3jyqRQlYUxP0 YU7fWGWjTHkRmnqKPknrMlg9c5V336NBvSpnCmNEZQGV6fmtp6iKTWElYC1ZhVWhNTp1 vonRsmoyxbJSna5BMoPquOUn4PwjkmTtfOBOn/20wn2HxU2OofVRrRk7gorsWx0x32lI bP4SUkmsnVTfz0r55p4sFDcpTsoddoETP+btXFJ7mRfcevRPVdpb89hlWrChcylPktq1 mqdw== X-Gm-Message-State: AOAM532ooIVPY4vf/LhJzkBomKjdKqoI0FTfygOmnRgRkgzXt06vbs/j VaDFGznLIsM6eZRzdZlC5Yw8B2tuUFi2Tg== X-Google-Smtp-Source: ABdhPJxn59rLZOxhPreENmFFdLg8Hx0NxQE6w7MoifCgJyrWBDHAKw0A9uvJlRB+1QwDIdwDcK/UxQ== X-Received: by 2002:a17:906:b24c:: with SMTP id ce12mr2143466ejb.89.1610100034267; Fri, 08 Jan 2021 02:00:34 -0800 (PST) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com. [209.85.128.41]) by smtp.gmail.com with ESMTPSA id b21sm3611514edr.53.2021.01.08.02.00.33 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 08 Jan 2021 02:00:34 -0800 (PST) Received: by mail-wm1-f41.google.com with SMTP id c133so7316620wme.4 for ; Fri, 08 Jan 2021 02:00:33 -0800 (PST) X-Received: by 2002:a1c:c308:: with SMTP id t8mr2392160wmf.22.1610100033143; Fri, 08 Jan 2021 02:00:33 -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> <79f59368-2295-c9d9-b09a-9d1256c7b0f2@collabora.com> In-Reply-To: <79f59368-2295-c9d9-b09a-9d1256c7b0f2@collabora.com> From: Tomasz Figa Date: Fri, 8 Jan 2021 19:00:21 +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 Wed, Dec 23, 2020 at 9:04 PM Helen Koike wrote: > > Hi Tomasz, > > On 12/21/20 12:13 AM, Tomasz Figa wrote: > > 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. > > Would this flag select between memory and color planes? > I didn't understand how this flag would be useful to S_FMT, could you > please clarify? I mean a flag that decides the plane layout between the 2 possible options (all planes in their own buffers at offsets 0 vs all planes in one buffer one after another), rather than giving too much flexibility for MMAP buffers, which isn't necessary any way, because DMABUF can be used if more flexibility is needed. Best regards, Tomasz > > Thanks > Helen > > > > >>> > >>>> > >>>> 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 > >