Re: [PATCH 02/13] media: rockchip: rga: extract helper to fill descriptors

[Michael Tretter <m.tretter@pengutronix.de>]

On Mon, 25 Sep 2023 09:32:49 +0200, Hans Verkuil wrote:
> On 14/09/2023 14:40, Michael Tretter wrote:
> > The IOMMU of the RGA is programmed with a list of DMA descriptors that
> > contain an 32 bit address per 4k page in the video buffers. The address
> > in the descriptor points to the start address of the page.
> > 
> > Introduce 'struct rga_dma_desc' to make the handling of the DMA
> > descriptors explicit instead of hiding them behind standard types.
> > 
> > As the descriptors only handle 32 bit addresses, addresses above 4 GB
> > cannot be addressed. If this is detected, stop filling the descriptor
> > list and report an error.
> > 
> > While at it, use provided helpers for iterating the sg_table instead of
> > manually calculating the DMA addresses.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/media/platform/rockchip/rga/rga-buf.c | 47 +++++++++++++++------------
> >  drivers/media/platform/rockchip/rga/rga.h     |  8 +++--
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
> > index 81508ed5abf3..df5ebc90e32d 100644
> > --- a/drivers/media/platform/rockchip/rga/rga-buf.c
> > +++ b/drivers/media/platform/rockchip/rga/rga-buf.c
> > @@ -5,6 +5,7 @@
> >   */
> >  
> >  #include <linux/pm_runtime.h>
> > +#include <linux/scatterlist.h>
> >  
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-ioctl.h>
> > @@ -15,6 +16,25 @@
> >  #include "rga-hw.h"
> >  #include "rga.h"
> >  
> > +static int fill_descriptors(struct rga_dma_desc *desc, struct sg_table *sgt)
> > +{
> > +	struct sg_dma_page_iter iter;
> > +	struct rga_dma_desc *tmp = desc;
> > +	unsigned int num_desc = 0;
> > +	dma_addr_t addr;
> > +
> > +	for_each_sgtable_dma_page(sgt, &iter, 0) {
> > +		addr = sg_page_iter_dma_address(&iter);
> > +		if (upper_32_bits(addr) != 0L)
> > +			return -1;
> > +		tmp->addr = lower_32_bits(addr);
> > +		tmp++;
> > +		num_desc++;
> > +	}
> > +
> > +	return num_desc;
> > +}
> > +
> >  static int
> >  rga_queue_setup(struct vb2_queue *vq,
> >  		unsigned int *nbuffers, unsigned int *nplanes,
> > @@ -114,11 +134,8 @@ void rga_buf_map(struct vb2_buffer *vb)
> >  {
> >  	struct rga_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> >  	struct rockchip_rga *rga = ctx->rga;
> > -	struct sg_table *sgt;
> > -	struct scatterlist *sgl;
> > -	unsigned int *pages;
> > -	unsigned int address, len, i, p;
> > -	unsigned int mapped_size = 0;
> > +	struct rga_dma_desc *pages;
> > +	unsigned int num_desc = 0;
> >  
> >  	if (vb->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> >  		pages = rga->src_mmu_pages;
> > @@ -126,23 +143,13 @@ void rga_buf_map(struct vb2_buffer *vb)
> >  		pages = rga->dst_mmu_pages;
> >  
> >  	/* Create local MMU table for RGA */
> > -	sgt = vb2_plane_cookie(vb, 0);
> > -
> > -	for_each_sg(sgt->sgl, sgl, sgt->nents, i) {
> > -		len = sg_dma_len(sgl) >> PAGE_SHIFT;
> > -		address = sg_phys(sgl);
> > -
> > -		for (p = 0; p < len; p++) {
> > -			dma_addr_t phys = address +
> > -					  ((dma_addr_t)p << PAGE_SHIFT);
> > -
> > -			pages[mapped_size + p] = phys;
> > -		}
> > -
> > -		mapped_size += len;
> > +	num_desc = fill_descriptors(pages, vb2_dma_sg_plane_desc(vb, 0));
> > +	if (num_desc < 0) {
> > +		dev_err(rga->dev, "Failed to map buffer");
> > +		return;
> >  	}
> >  
> >  	/* sync local MMU table for RGA */
> >  	dma_sync_single_for_device(rga->dev, virt_to_phys(pages),
> > -				   8 * PAGE_SIZE, DMA_BIDIRECTIONAL);
> > +				   num_desc * sizeof(*pages), DMA_BIDIRECTIONAL);
> >  }
> > diff --git a/drivers/media/platform/rockchip/rga/rga.h b/drivers/media/platform/rockchip/rga/rga.h
> > index 5fa9d2f366dc..22f7da28ac51 100644
> > --- a/drivers/media/platform/rockchip/rga/rga.h
> > +++ b/drivers/media/platform/rockchip/rga/rga.h
> > @@ -40,6 +40,10 @@ struct rga_frame {
> >  	u32 size;
> >  };
> >  
> > +struct rga_dma_desc {
> > +	u32 addr;
> > +};
> > +
> >  struct rockchip_rga_version {
> >  	u32 major;
> >  	u32 minor;
> > @@ -81,8 +85,8 @@ struct rockchip_rga {
> >  	struct rga_ctx *curr;
> >  	dma_addr_t cmdbuf_phy;
> >  	void *cmdbuf_virt;
> > -	unsigned int *src_mmu_pages;
> > -	unsigned int *dst_mmu_pages;
> > +	struct rga_dma_desc *src_mmu_pages;
> > +	struct rga_dma_desc *dst_mmu_pages;
> 
> This breaks compilation:
> 
> drivers/media/platform/rockchip/rga/rga.c: In function ‘rga_probe’:
> drivers/media/platform/rockchip/rga/rga.c:875:28: error: assignment to ‘struct rga_dma_desc *’ from incompatible pointer type ‘unsigned int *’ [-Werror=incompatible-pointer-types]
>   875 |         rga->src_mmu_pages =
>       |                            ^
> drivers/media/platform/rockchip/rga/rga.c:881:28: error: assignment to ‘struct rga_dma_desc *’ from incompatible pointer type ‘unsigned int *’ [-Werror=incompatible-pointer-types]
>   881 |         rga->dst_mmu_pages =
>       |                            ^
> 
> It's probably something that is fixed in a later patch, but as it is right
> now this series breaks bisect.

Thanks. I'll fix this in v2.

Michael

> 
> Regards,
> 
> 	Hans
> 
> >  };
> >  
> >  struct rga_frame *rga_get_frame(struct rga_ctx *ctx, enum v4l2_buf_type type);
> > 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel