Re: [PATCH v1] media: videobuf2: Allow applications customize data offsets of capture buffers

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 3/23/22 22:21, Nicolas Dufresne wrote:
> Le mercredi 23 mars 2022 à 17:28 +0300, Dmitry Osipenko a écrit :
>> Hi Nicolas,
>>
>> On 3/23/22 16:05, Nicolas Dufresne wrote:
>>> Hi Dmitry,
>>>
>>> thanks for giving a second look a this issue.
>>>
>>> Le mardi 22 mars 2022 à 16:23 +0300, Dmitry Osipenko a écrit :
>>>> Use data offsets provided by applications for multi-planar capture
>>>> buffers. This allows V4L to import and use dma-bufs exported by other
>>>> subsystems in cases where application wants to customize data offsets
>>>> of capture buffers in order to meet hardware alignment requirements of
>>>> both dma-buf exporter and importer.
>>>>
>>>> This feature is wanted for providing a better support of media hardware
>>>> found on Chromebooks. In particular display and camera ISP hardware of
>>>> Rockchip and MediaTek SoCs require special handling by userspace because
>>>> display h/w has specific alignment requirements that don't match default
>>>> alignments expected by V4L and there is a need to customize the data
>>>> offsets in case of multi-planar formats.
>>>>
>>>> Some drivers already have preliminary support for data offsets
>>>> customization of capture buffers, like NVIDIA Tegra video decoder driver
>>>> for example, and V4L allows applications to provide data offsets for
>>>> multi-planar output buffers, let's support such customization for the
>>>> capture buffers as well.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>>>  Documentation/userspace-api/media/v4l/buffer.rst | 9 ++++++++-
>>>>  drivers/media/common/videobuf2/videobuf2-v4l2.c  | 7 +++++++
>>>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/buffer.rst b/Documentation/userspace-api/media/v4l/buffer.rst
>>>> index 4638ec64db00..75b1929e2acb 100644
>>>> --- a/Documentation/userspace-api/media/v4l/buffer.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/buffer.rst
>>>> @@ -369,13 +369,20 @@ struct v4l2_plane
>>>>        - ``data_offset``
>>>>        - Offset in bytes to video data in the plane. Drivers must set this
>>>>  	field when ``type`` refers to a capture stream, applications when
>>>> -	it refers to an output stream.
>>>> +	it refers to an output or capture stream.
>>>
>>> There is a clear contradiction in this paragraph. Both the driver and the
>>> application MUST set the data_offset.
>>
>> I'm not sure where the contradiction is. Application must initialize the
>> data_offset and driver must set data_offset too, if it's appropriate to
>> do that for a particular driver.
>>
>>> Would it be possible to demo your idea by implementing this in a virtual driver
>>> ? vivid already have data_offset for capture in some cases, you could verify if
>>> your idea works without any conflict in this scenario.
>>
>> I actually considered implementing it in the vivid driver, but vivid
>> driver already sets the data_offset to fixed values [1], so I decided
>> that not to change it.
>>
>> But maybe we actually could extend the vivid driver by accepting
>> data_offset from userspace for the cases where the fixed offset value is
>> zero in the driver.. not sure.
> 
> The is the core of the issue, how do you represent both a driver use of
> data_offset and a userland provided data_offset at the same time. Contradiction
> might be the wrong term, but minimally there is a large gap in the specification
> for which I don't have an easy answer.

In the doc-comment I said: "Handling of application's offsets is
driver-dependent, application must use the resulting buffer offset."

This can be made stricter using a new
V4L2_BUF_FLAG_USE_CAPTURE_MPLANE_DATA_OFFSET, for example. But then it
doesn't feel good anymore to me because output buffers don't require a
special flag, hence it feels inconsistent.

There is another variant with adding new data_offset fields to the
v4l2_plane_pix_format and then will be possible to set data offsets
using VIDIOC_S_FMT for both S/MPLANEs, assuming that we won't need more
than 3 offsets for SPLANEs. But this is a much more invasive and
questionable UAPI extension.

--- >8 ---
@@ -2268,12 +2268,14 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
  *                     this plane will be used
  * @bytesperline:      distance in bytes between the leftmost pixels in two
  *                     adjacent lines
+ * @data_offset                offset in bytes to the beginning of the
plane's data
  * @reserved:          drivers and applications must zero this array
  */
 struct v4l2_plane_pix_format {
        __u32           sizeimage;
        __u32           bytesperline;
-       __u16           reserved[6];
+       __u32           data_offset[3];
 } __attribute__ ((packed));
--- >8 ---

The root of the problem is that DRM UAPI is more flexible and allows to
customize offsets for both S/MPLANEs, while V4L doesn't allow to do it
at all. I'm exploring all the potential options, so far neither of the
proposed variants is ideal.