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@xxxxxxxxxxxxx/ >>> >>> >>> 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(-) >>> >>
