[RFC PATCH] media: hantro: respect data_offset in vb2_plane

[Nicolas Dufresne]

Hi Micheal,

Le lundi 27 mars 2023 à 15:23 +0200, Michael Tretter a écrit :
> The vb2_plane in the vb2_v4l2_buffer may have a data_offset, which is
> written by user space to tell the driver that the data starts at an
> offset into the buffer. Currently the hantro driver assumes that the
> data starts directly at the base address of the buffer.
> 
> Add the data_offset to the plane dma_address to make sure that the
> encoder actually reads the plane data if the user space put the plane
> data at an offset into the buffer. Otherwise the encoded data may not be
> the data that userspace expected to be encoded.
> 

The data_offset for this purpose have limited use, and this usage is only valid
for encoders (OUTPUT queues). Would it be possible to state this clearly in the
subject ?

> 
> Signed-off-by: Michael Tretter <m.tretter at pengutronix.de>
> ---
> Hi,
> 
> Most other drivers also assume that the address returned by
> vb2_dma_contig_plane_dma_addr() is the start of the plane data. Maybe it
> would be better to change vb2_dma_contig_plane_dma_addr() to already add
> the data_offset to the plane address. However, there are a few drivers
> that already have a helper that respects the data_offset, but that seems
> to be the exception rather than the rule.

I had this discussion recently, and its a bit hardware specific. Some HW may
support any random offset, in which case you will program the original (and
aligned) address, and program the offset in the HW. But some HW may not, in
which case you need to add alignment validation in the driver.

> 
> What I am actually trying to achieve is to import a V4L2_PIX_FMT_NV12
> buffer from a Rockchip RGA2 (which doesn't support the multi-planar API)
> as a V4L2_PIX_FMT_NV12M buffer into the Hantro JPEG encoder (which
> doesn't support V4L2_PIX_FMT_NV12). Solving this by importing the same
> FD for each plane with a respective offset is how one would import such
> a buffer with the DRM API. Please tell me, if my approach is wrong and,
> if so, how I should solve it differently.

The approach is fine, this is the only valid usage of data_offset when passed by
userspace. Though, you are making your live much more difficult then needed,
adding NV12 (contiguous) support to the jpeg encoder should be fairly easy too.

> 
> Michael
> ---
>  .../verisilicon/rockchip_vpu2_hw_jpeg_enc.c   | 24 +++++++++++++------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> index 8395c4d48dd0..05df7768187d 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> @@ -32,6 +32,16 @@
>  
>  #define VEPU_JPEG_QUANT_TABLE_COUNT 16
>  
> +static dma_addr_t rockchip_vpu2_plane_dma_addr(struct vb2_buffer *vb,
> +					       unsigned int plane_no)
> +{
> +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> +	dma_addr_t base = vb2_dma_contig_plane_dma_addr(vb, plane_no);
> +	unsigned int offset = v4l2_buf->planes[plane_no].data_offset;
> +
> +	return base + offset;

As the offset is not being passed as a control to the HW, you need to go back to
the datasheet, and figure-out what is the required alignment. This aligment then
needs to be validated. I don't know the exact answer for you, we don't check the
dma address because they are all page aligned, so its not needed.

If the alignment is not acceptable, you have to fail synchronously in
VIDIOC_QBUF, so that userspace knows and can fallback to copy.

> +}
> +
>  static void rockchip_vpu2_set_src_img_ctrl(struct hantro_dev *vpu,
>  					   struct hantro_ctx *ctx)
>  {
> @@ -79,23 +89,23 @@ static void rockchip_vpu2_jpeg_enc_set_buffers(struct hantro_dev *vpu,
>  
>  	WARN_ON(pix_fmt->num_planes > 3);
>  
> -	vepu_write_relaxed(vpu, vb2_dma_contig_plane_dma_addr(dst_buf, 0) +
> +	vepu_write_relaxed(vpu, rockchip_vpu2_plane_dma_addr(dst_buf, 0) +
>  				ctx->vpu_dst_fmt->header_size,
>  			   VEPU_REG_ADDR_OUTPUT_STREAM);
>  	vepu_write_relaxed(vpu, size_left, VEPU_REG_STR_BUF_LIMIT);
>  
>  	if (pix_fmt->num_planes == 1) {
> -		src[0] = vb2_dma_contig_plane_dma_addr(src_buf, 0);
> +		src[0] = rockchip_vpu2_plane_dma_addr(src_buf, 0);
>  		vepu_write_relaxed(vpu, src[0], VEPU_REG_ADDR_IN_PLANE_0);
>  	} else if (pix_fmt->num_planes == 2) {
> -		src[0] = vb2_dma_contig_plane_dma_addr(src_buf, 0);
> -		src[1] = vb2_dma_contig_plane_dma_addr(src_buf, 1);
> +		src[0] = rockchip_vpu2_plane_dma_addr(src_buf, 0);
> +		src[1] = rockchip_vpu2_plane_dma_addr(src_buf, 1);
>  		vepu_write_relaxed(vpu, src[0], VEPU_REG_ADDR_IN_PLANE_0);
>  		vepu_write_relaxed(vpu, src[1], VEPU_REG_ADDR_IN_PLANE_1);
>  	} else {
> -		src[0] = vb2_dma_contig_plane_dma_addr(src_buf, 0);
> -		src[1] = vb2_dma_contig_plane_dma_addr(src_buf, 1);
> -		src[2] = vb2_dma_contig_plane_dma_addr(src_buf, 2);
> +		src[0] = rockchip_vpu2_plane_dma_addr(src_buf, 0);
> +		src[1] = rockchip_vpu2_plane_dma_addr(src_buf, 1);
> +		src[2] = rockchip_vpu2_plane_dma_addr(src_buf, 2);
>  		vepu_write_relaxed(vpu, src[0], VEPU_REG_ADDR_IN_PLANE_0);
>  		vepu_write_relaxed(vpu, src[1], VEPU_REG_ADDR_IN_PLANE_1);
>  		vepu_write_relaxed(vpu, src[2], VEPU_REG_ADDR_IN_PLANE_2);