Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a7c858461b99de2d4afad22d888acc3a74850240.camel@ndufresne.ca>
Subject: Re: [PATCH v1] media: videobuf2: Allow applications customize data
 offsets of capture buffers
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Tomasz Figa <tfiga@chromium.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        Gustavo Padovan <gustavo.padovan@collabora.com>,
        Boris Brezillon <bbrezillon@collabora.com>,
        Daniel Almeida <daniel.almeida@collabora.com>,
        Sebastian Fricke <sebastian.fricke@collabora.com>,
        Laura Nao <laura.nao@collabora.com>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Dmitry Osipenko <digetx@gmail.com>
Date:   Wed, 23 Mar 2022 09:05:35 -0400
In-Reply-To: <20220322132329.6527-1-dmitry.osipenko@collabora.com>
References: <20220322132329.6527-1-dmitry.osipenko@collabora.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

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.

>  
>  	.. note::
>  
>  	   That data_offset is included  in ``bytesused``. So the
>  	   size of the image in the plane is ``bytesused``-``data_offset``
>  	   at offset ``data_offset`` from the start of the plane.
> +
> +	   For capture planes data_offset may be specified by applications
> +	   and by drivers. Driver may override application's offset or error
> +	   out buffer if offset can't be satisfied by hardware. This allows
> +	   applications to customize data offsets of imported dma-bufs.
> +	   Handling of application's offsets is driver-dependent, application
> +	   must use the resulting buffer offset.
>      * - __u32
>        - ``reserved[11]``
>        - Reserved for future use. Should be zeroed by drivers and
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6edf4508c636..929107a431cc 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -263,6 +263,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>  						psrc->bytesused : pdst->length;
>  				pdst->data_offset = psrc->data_offset;
>  			}
> +		} else {
> +			for (plane = 0; plane < vb->num_planes; ++plane) {
> +				struct vb2_plane *pdst = &planes[plane];
> +				struct v4l2_plane *psrc = &b->m.planes[plane];
> +
> +				pdst->data_offset = psrc->data_offset;
> +			}
>  		}
>  	} else {
>  		/*