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

[Helen Koike <helen.koike@collabora.com>]

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?

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
>