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

[Nicolas Dufresne]

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.

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 {
>  		/*