Re: [PATCH v7 03/28] media: videobuf2: calculate restricted memory size

[Hans Verkuil]

On 20/07/2024 09:15, Yunfei Dong wrote:
> Getting the physical address with sg_dma_address for restricted memory,
> only return the first physical address size since sg may not be physical
> continuous, then leading to the dmabuf size is small than buf size. Need
> to bypass continuous checking for restricted memory.
> 
> Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> ---
>  .../common/videobuf2/videobuf2-dma-contig.c   | 34 +++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index 3d4fd4ef5310..f0e4652b615f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -66,6 +66,22 @@ static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
>  	return size;
>  }
>  
> +/**************************************************/
> +/*   restricted mem scatterlist table functions   */
> +/**************************************************/
> +
> +static unsigned long vb2_dc_get_res_mem_contiguous_size(struct sg_table *sgt)
> +{
> +	struct scatterlist *s;
> +	unsigned int i;
> +	unsigned long size = 0;
> +
> +	for_each_sgtable_dma_sg(sgt, s, i)
> +		size += sg_dma_len(s);
> +
> +	return size;
> +}

I think it is better to add a 'bool restricted' argument to vb2_dc_get_contiguous_size.
If true, then skip the 'expected' check there.

> +
>  /*********************************************/
>  /*         callbacks for all buffers         */
>  /*********************************************/
> @@ -648,10 +664,13 @@ static void *vb2_dc_get_userptr(struct vb2_buffer *vb, struct device *dev,
>  		goto fail_sgt_init;
>  	}
>  
> -	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (buf->vb->vb2_queue->restricted_mem)

I think it is better to do the same as with buf->non_coherent_mem,
so add a 'bool restricted_mem' to struct vb2_dc_buf and set it in
vb2_dc_alloc(). It makes this code easier to read.

> +		contig_size = vb2_dc_get_res_mem_contiguous_size(sgt);
> +	else
> +		contig_size = vb2_dc_get_contiguous_size(sgt);
>  	if (contig_size < size) {
> -		pr_err("contiguous mapping is too small %lu/%lu\n",
> -			contig_size, size);
> +		pr_err("contiguous mapping is too small %lu/%lu/%u\n",
> +		       contig_size, size, buf->vb->vb2_queue->restricted_mem);

Rather than add a "/%u", which is not easy to understand, perhaps do this:

		pr_err("%scontiguous mapping is too small %lu/%lu\n",
		       buf->vb->vb2_queue->restricted_mem ? "restricted " : "",
		       contig_size, size);

>  		ret = -EFAULT;
>  		goto fail_map_sg;
>  	}
> @@ -711,10 +730,13 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
>  	}
>  
>  	/* checking if dmabuf is big enough to store contiguous chunk */
> -	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (buf->vb->vb2_queue->restricted_mem)
> +		contig_size = vb2_dc_get_res_mem_contiguous_size(sgt);
> +	else
> +		contig_size = vb2_dc_get_contiguous_size(sgt);
>  	if (contig_size < buf->size) {
> -		pr_err("contiguous chunk is too small %lu/%lu\n",
> -		       contig_size, buf->size);
> +		pr_err("contiguous chunk is too small %lu/%lu/%u\n",
> +		       contig_size, buf->size, buf->vb->vb2_queue->restricted_mem);

Ditto.

>  		dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt,
>  						  buf->dma_dir);
>  		return -EFAULT;

Regards,

	Hans