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

[Michael Tretter]

Hi Nicolas,

On Fri, 31 Mar 2023 11:07:44 -0400, Nicolas Dufresne wrote:
> 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 ?

I'll update 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.

This makes sense, but it's still a bit surprising that some drivers don't
respect the data_offset. Maybe there should be a check that warns or returns
an error to userspace if the data_offset is set but ignored by the driver?
With such an error, the behavior of vb2_dma_contig_plane_dma_addr() would be
obvious. OTOH, the warning would have to be added in the driver, too.

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

It is fairly easy to add this in the driver, but the negotiation in GStreamer
is becomes a bit cumbersome as the caps don't distinguish between
V4L2_PIX_FMT_NV12 and V4L2_PIX_FMT_NV12M and the encoder will prefer
V4L2_PIX_FMT_NV12M if NV12 is negotiated. Letting userspace explicitly state
the offset of the second plane makes things easier as well. Thus, I discarded
the option of adding NV12 contiguous support to this encoder.

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

I will add the necessary checks and send a proper version of the patch.

Thanks a lot!

Michael

> 
> > +}
> > +
> >  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);
> 
>