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

[Nicolas Dufresne]

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@xxxxxxxxxxxxx>
> > > ---
> > >  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.

> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/media/test-drivers/vivid/vivid-vid-cap.c#L172
> 
> I verified my idea using the NVIDIA Tegra video decoder driver, which
> already takes data_offsets for capture planes [3] and libvdpau-tegra
> imports DRM dma-bufs into the V4L driver [4][5].
> 
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/media/platform/nvidia/tegra-vde/v4l2.c#n236
> [4]
> https://github.com/grate-driver/libvdpau-tegra/blob/master/src/decoder.c#L685
> [5]
> https://github.com/grate-driver/libvdpau-tegra/blob/master/src/v4l2.c#L359
> 
> The plan is to extend RK ISP driver with support of data_offsets next,
> once we'll agree that this acceptable approach and we don't actually
> need go back to implementing the new VIDIOC_EXT_QBUF ioctl [6].
> 
> [6]
> https://patchwork.linuxtv.org/project/linux-media/cover/20210114180738.1758707-1-helen.koike@xxxxxxxxxxxxx/
> 
> This patch solves the problem for userspace when it wants to import
> buffers into V4L in case of multi-planar formats, but it doesn't cover
> all other possible cases that may require offsets customization too. On
> the other hand, it's easy to start accepting data_offset for the capture
> MPLANES without introducing new UAPIs, so I decided that will be best to
> start easy with the simplest solution.