Re: [PATCH v5 2/7] media: v4l2: Add extended buffer operations

[Tomasz Figa <tfiga@chromium.org>]

On Wed, Dec 23, 2020 at 9:04 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Tomasz,
>
> On 12/21/20 12:13 AM, Tomasz Figa wrote:
> > On Thu, Dec 17, 2020 at 10:20 PM Helen Koike <helen.koike@collabora.com> 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 <helen.koike@collabora.com> wrote:
> >>>>
> >>>> Hi Tomasz,
> >>>>
> >>>> On 12/14/20 7:46 AM, Tomasz Figa wrote:
> >>>>> On Fri, Dec 4, 2020 at 4:52 AM Helen Koike <helen.koike@collabora.com> 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 <helen.koike@collabora.com> 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
> >