Message-ID: <81c775c6-451f-8973-32cb-9e7a70034386@xs4all.nl>
Date:   Tue, 21 Jul 2020 16:34:49 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Helen Koike <helen.koike@...labora.com>, mchehab@...nel.org,
        hans.verkuil@...co.com, laurent.pinchart@...asonboard.com,
        sakari.ailus@....fi, linux-media@...r.kernel.org
Cc:     tfiga@...omium.org, hiroh@...omium.org, nicolas@...fresne.ca,
        Brian.Starkey@....com, kernel@...labora.com,
        boris.brezillon@...labora.com, narmstrong@...libre.com,
        linux-kernel@...r.kernel.org, frkoenig@...omium.org,
        mjourdan@...libre.com, stanimir.varbanov@...aro.org
Subject: Re: [PATCH v4 0/6] media: v4l2: Add extended fmt and buffer ioctls

On 21/07/2020 16:23, Helen Koike wrote:
> Hi,
> 
> On 7/21/20 7:24 AM, Hans Verkuil wrote:
>> On 17/07/2020 13:54, Helen Koike wrote:
>>> Hi,
>>>
>>> I'm sorry for taking too long to submit v4.
>>>
>>> It is not perfect, not all v4l2-compliance tests passes, but I'd like a review,
>>> specially on the API and potential problems, so I can focus on improving implementation
>>> and maybe drop the RFC tag for next version.
>>>
>>> Follow below what changed in v4 and some items I'd like to discuss:
>>>
>>>
>>> * Ioctl to replace v4l2_pix_format
>>> ---------------------------------------------------------------------------------
>>> During last media summit, we agreed to create ioctls that replace the v4l2_pix_format
>>> struct and leave the other structs in the v4l2_format union alone.
>>> Thus I refactored the code to receive struct v4l2_ext_pix_format, and I renamed the
>>> ioctls, so now we have:
>>>
>>> int ioctl(int fd, VIDIOC_G_EXT_FMT, struct v4l2_ext_pix_format *argp);
>>> int ioctl(int fd, VIDIOC_S_EXT_FMT, struct v4l2_ext_pix_format *argp);
>>> int ioctl(int fd, VIDIOC_TRY_EXT_FMT, struct v4l2_ext_pix_format *argp);
>>>
>>> The only valid types are V4L2_BUF_TYPE_VIDEO_CAPTURE and V4L2_BUF_TYPE_VIDEO_OUTPUT,
>>> all the other types are invalid with this API.
>>>
>>>
>>> * Modifiers
>>> ---------------------------------------------------------------------------------
>>> I understand that unifying DRM and V4L2 pixel formats is not possible, but I'd like
>>> to unify the modifiers [1].
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_fourcc.h#n290
>>>
>>> Should we use the DRM modifiers directly in the V4L2 API?
>>
>> For now, yes. Most of the modifier work is done in DRM, it is only fairly recent
>> that the media subsystem starts to have a need for it. So for now just use the drm
>> header and prefixes.
> 
> ack
> 
>>
>>> Or should we move this header to a common place and change the prefix? (which requires
>>> us to sync with DRM community).
>>> Or should we create a v4l2 header, defining V4L2_ prefixed macros mapping to DRM_
>>> macros?
>>>
>>> For now, patch 1/6 includes drm/drm_fourcc.h and it is using DRM_FORMAT_MOD_*
>>>
>>> As discussed before, It would be nice to have documentation describing DRM fourcc
>>> equivalents (I'm not sure if someone started this already), listing the number of
>>> planes per format.
>>>
>>> We should also document which pixelformats are valid for the EXT_API, since multiplanar
>>> and tile versions like V4L2_PIX_FMT_NV12MT_16X16 (which seems equivalent to
>>> DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, and could have a more generic name) should be
>>> replaced by a modifier.
>>>
>>> Regarding flags [2] field in struct v4l2_pix_format_mplane [3]:
>>> The only defined flag is V4L2_PIX_FMT_FLAG_PREMUL_ALPHA, and it is only used by vsp1 driver.
>>> Which I believe could be replaced by a modifier, to avoid another field that changes
>>> pixel formats, so I removed it from the EXT API (we can always add it back later with
>>> the reserved fields).
>>
>> The colorspace series that Dafna is working on will add a V4L2_PIX_FMT_FLAG_SET_CSC
>> flag, so this flags field will be needed.
> 
> This was because the CSC fields were defined in the API as read only (filled by the driver),
> what if those fields in struct v4l2_ext_pix_format allows user to change the CSC fields,
> and it will just fill the right one if it is not supported (similar to how other fields works
> already).
> Please, let me know if I'm missing something.

Ah, that's true, I forgot about that.

> 
>>
>>>
>>> [2] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-reserved.html#format-flags
>>> [3] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/pixfmt-v4l2-mplane.html?highlight=v4l2_pix_format_mplane#c.v4l2_pix_format_mplane
>>>
>>> We also discussed to add a new ENUM_FMT_EXT ioctl to return all pixelformats + modifiers
>>> combinations. I still didn't add it in this version, but I don't think it affects
>>> what is in this RFC and it can be added later.
>>>
>>>
>>> * Buffers/Plane offset
>>> ---------------------------------------------------------------------------------
>>>
>>> My understanding is that inside a memory buffer we can have multiple planes in random
>>> offsets.
>>> I was comparing with the DRM API [4], where it can have the same dmabuf for multiple
>>> planes in different offsets, and I started to think we could simplify our API, so
>>> I took the liberty to do some more changes, please review struct v4l2_ext_plane in
>>> this RFC.
>>>
>>> I removed the data_offset, since it is unused (See Laurent's RFC repurposing this
>>> field [5]). And comparing to the DRM API, it seems to me we only need a single offset
>>> field.
>>>
>>> We could also check about overlapping planes in a memory buffer, but this is complicated
>>> if we use the same memory buffer with different v4l2_ext_buffer objects. We can also leave
>>> to the driver to check situations that may cause HW errors.
>>>
>>> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/drm/drm_mode.h#n489
>>> [5] https://patchwork.linuxtv.org/patch/29177/
>>>
>>>
>>> * Multistream Channels
>>> ---------------------------------------------------------------------------------
>>> During last media summit, we discussed about adding a channel number to the API to
>>> support multistreams. i.e, to have multiple queues through a single video node.
>>>
>>> Use cases:
>>>
>>>     - Blitters: can take multiple streams as input, which would require multiple OUTPUT queues.
>>>
>>>     As Nicolas was explaining me:
>>>     "The blitters comes with a lot of variation between hardware. Most blitters at
>>>     least support 3 frames buffer. 2 inputs and one output. The second input is usually
>>>     optional, as the output buffer data is not always overwritten (e.g. SRC_OVER
>>>     blend or 1 input). Some of them have additional solid color or pattern that can
>>>     be used too. Advanced blitters will have composition feature, and may support more
>>>     input buffers to reduce the added latency that would be normally done through cascading
>>>     the operations. Note that each input can have different size and different cropping
>>>     region. Many blitters can scale and render to a sub-region of the CAPTURE buffer."
>>>
>>>     - Multis-calers: can produce multiple streams, which would require multiple CAPTURE queues.
>>>
>>>     As Nicolas was explaining me:
>>>     "This type of HW (or soft IP) is commonly found on HW used to produce internet
>>>     streams for fragmented and scalable protocols (HLS, DASH).  Basically they are
>>>     used to transform one stream into multiple sized streams prior from being encoded."
>>>
>>> Modeling as channels allows the API to have synchronized Start/Stop between queues,
>>> and also avoid the complexity of using the Media API in a topology with multiple video
>>> nodes, which complicates userspace.
>>>
>>> This requires adding a new channel id in ioctls for formats (G_FMT/S_FMT/TRY_FMT), and
>>> also for buffers (QBUF/DBUF).
>>> We also need a mechanism to enumerate channels and their properties.
>>> Since we don't have a clear view how this would work, for now I'm leaving reserved bits
>>> in the structs, so we can add them later.
>>>
>>>
>>> * Timecode
>>> ---------------------------------------------------------------------------------
>>> During last media summit, we discussed to return the v4l2_timecode field to the API,
>>> since Nicolas mentioned that, even if it is not used by any upstreamed driver, it
>>> is used by out-of-tree drivers.
>>>
>>> I've been discussing with Nicolas about this, and we can avoid adding too many metadata
>>> to the buffer struct by using the Read-Only Request API [6] for retrieving more information
>>> when required, similar to HDR.
>>>
>>> The RO Request API has the ability to read a control using a request that has already
>>> completed, the control value lives as long as the request object. If it's not read
>>> (or if there was no request), the data is simply ignored/discard.
>>>
>>> Since no upstream driver uses the timecode field, there are no conversions that need
>>> to be done.
>>
>> That's a reasonable solution.
>>
>>>
>>> [6] https://patchwork.kernel.org/cover/11635927/
>>>
>>>
>>> * Other changes (and some questions) in this version:
>>> ---------------------------------------------------------------------------------
>>> - Added reserved fields to struct
>>>
>>> - The only difference between previously proposed VIDIOC_EXT_EXPBUF and VIDIOC_EXPBUF,
>>> was that with VIDIOC_EXT_EXPBUF we can export multiple planes at once. I think we
>>> can add this later, so I removed it from this RFC to simplify it.
>>>
>>> - v4l2_buffer [7] has a memory field (enum v4l2_memory [8]). We kept this field in
>>> struct v4l2_ext_buffer, buf I was wondering if this shouldn't be in struct v4l2_ext_plane
>>> instead.
>>
>> This pops up every so often. The only use-case I can think of is when you return both
>> video planes and metadata planes where the metadata might be MMAP and the video planes
>> DMABUF. But it would add quite a bit of complexity, I suspect.
> 
> We could move it to struct v4l2_ext_plane to not limit the API, but for now, only allowing
> a single memory type for all planes. So we don't need to extend the struct later if we see
> a need for this. What do you think?

Go with that for now, yes.

Regards,

	Hans

> 
>>
>>>
>>> [7] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/buffer.html?highlight=v4l2_buffer#c.v4l2_buffer
>>> [8] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/buffer.html?highlight=v4l2_memory#c.v4l2_memory
>>>
>>> - In struct v4l2_ext_pix_format, we have:
>>>
>>>         struct v4l2_plane_ext_pix_format plane_fmt[VIDEO_MAX_PLANES];
>>>
>>> The number of planes can be deducted from plane_fmt[i].sizeimage != 0, so I removed
>>> the num_planes field. Please let me know if we can't use sizeimage for this.
>>> In DRM, we know the number of planes from drm_mode_fb_cmd2 by the number of handle
>>> args passed which are not 0.
>>> This also avoids num_planes to be bigger then VIDEO_MAX_PLANES.
>>
>> I have no objection to this. You do probably need to add a note about there not
>> being holes, e.g. plane_fmt[0].sizeimage is != 0, so is plane_fmt[2].sizeimage,
>> but plane_fmt[1].sizeimage == 0. That's likely something you don't want.
> 
> ack
> 
> 
> Regards,
> Helen
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> - Added flags field to struct v4l2_ext_create_buffers
>>>
>>>
>>> * Fixed bugs here and there
>>> ---------------------------------------------------------------------------------
>>> I fixed some bugs found with v4l2-compliance (not all of them yet),
>>> through script v4l-utils/contrib/test/test-media.
>>>
>>> I adapted what Boris did for v4l-utils in previous version to this version:
>>> https://gitlab.collabora.com/koike/v4l-utils/-/tree/ext-api/wip
>>>
>>> Boris' questions regarding DMABUF in last version still holds [9].
>>>
>>> [9] https://patchwork.linuxtv.org/project/linux-media/cover/20191008091119.7294-1-boris.brezillon@collabora.com/
>>>
>>>
>>> Please, let me know your feedback,
>>> Helen
>>>
>>>
>>> Boris Brezillon (5):
>>>   media: v4l2: Extend pixel formats to unify single/multi-planar
>>>     handling (and more)
>>>   media: videobuf2: Expose helpers to implement the _ext_fmt and
>>>     _ext_buf hooks
>>>   media: mediabus: Add helpers to convert a ext_pix format to/from a
>>>     mbus_fmt
>>>   media: vivid: Convert the capture and output drivers to
>>>     EXT_FMT/EXT_BUF
>>>   media: vimc: Implement the ext_fmt and ext_buf hooks
>>>
>>> Hans Verkuil (1):
>>>   media: v4l2: Add extended buffer operations
>>>
>>>  .../media/common/videobuf2/videobuf2-core.c   |   2 +
>>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 549 +++++-----
>>>  .../media/test-drivers/vimc/vimc-capture.c    |  61 +-
>>>  drivers/media/test-drivers/vimc/vimc-common.c |   6 +-
>>>  drivers/media/test-drivers/vimc/vimc-common.h |   2 +-
>>>  drivers/media/test-drivers/vivid/vivid-core.c |  70 +-
>>>  .../test-drivers/vivid/vivid-touch-cap.c      |  26 +-
>>>  .../test-drivers/vivid/vivid-touch-cap.h      |   3 +-
>>>  .../media/test-drivers/vivid/vivid-vid-cap.c  | 169 +---
>>>  .../media/test-drivers/vivid/vivid-vid-cap.h  |  15 +-
>>>  .../media/test-drivers/vivid/vivid-vid-out.c  | 193 ++--
>>>  .../media/test-drivers/vivid/vivid-vid-out.h  |  15 +-
>>>  drivers/media/v4l2-core/v4l2-dev.c            |  50 +-
>>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 934 ++++++++++++++++--
>>>  include/media/v4l2-ioctl.h                    |  60 ++
>>>  include/media/v4l2-mediabus.h                 |  42 +
>>>  include/media/videobuf2-core.h                |   6 +-
>>>  include/media/videobuf2-v4l2.h                |  21 +-
>>>  include/uapi/linux/videodev2.h                | 144 +++
>>>  19 files changed, 1650 insertions(+), 718 deletions(-)
>>>
>>

Powered by blists - more mailing lists