Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver

From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org
Cc: "Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Naushir Patuck" <naush@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.org>
Subject: Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver
Date: Mon, 11 May 2020 20:42:57 +0200	[thread overview]
Message-ID: <9b42ad8c4c39ac3873e7c3ea2951bea1caef8bd1.camel@suse.de> (raw)
In-Reply-To: <20200504092611.9798-7-laurent.pinchart@ideasonboard.com>

[-- Attachment #1: Type: text/plain, Size: 5368 bytes --]

Hi Dave,
some questions/comments.

On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> 
> Add Broadcom VideoCore Shared Memory support.
> 
> This new driver allows contiguous memory blocks to be imported
> into the VideoCore VPU memory map, and manages the lifetime of
> those objects, only releasing the source dmabuf once the VPU has
> confirmed it has finished with it.
> 
> Driver upported from the RaspberryPi BSP at revision:
> 890691d1c996 ("staging: vc04_services: Fix vcsm overflow bug when counting
> transactions")
> forward ported to recent mainline kernel version.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---

[...]

> +
> +/* Import a dma_buf to be shared with VC. */
> +int
> +vc_sm_cma_import_dmabuf_internal(struct vc_sm_privdata_t *private,
> +				 struct dma_buf *dma_buf,
> +				 int fd,
> +				 struct dma_buf **imported_buf)
> +{
> +	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +	struct vc_sm_buffer *buffer = NULL;
> +	struct vc_sm_import import = { };
> +	struct vc_sm_import_result result = { };
> +	struct dma_buf_attachment *attach = NULL;
> +	struct sg_table *sgt = NULL;
> +	dma_addr_t dma_addr;
> +	int ret = 0;
> +	int status;
> +
> +	/* Setup our allocation parameters */
> +	pr_debug("%s: importing dma_buf %p/fd %d\n", __func__, dma_buf, fd);
> +
> +	if (fd < 0)
> +		get_dma_buf(dma_buf);
> +	else
> +		dma_buf = dma_buf_get(fd);
> +
> +	if (!dma_buf)
> +		return -EINVAL;
> +
> +	attach = dma_buf_attach(dma_buf, &sm_state->pdev->dev);
> +	if (IS_ERR(attach)) {
> +		ret = PTR_ERR(attach);
> +		goto error;
> +	}
> +
> +	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> +	if (IS_ERR(sgt)) {
> +		ret = PTR_ERR(sgt);
> +		goto error;
> +	}
> +
> +	/* Verify that the address block is contiguous */
> +	if (sgt->nents != 1) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	/* Allocate local buffer to track this allocation. */
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	import.type = VC_SM_ALLOC_NON_CACHED;
> +	dma_addr = sg_dma_address(sgt->sgl);
> +	import.addr = (u32)dma_addr;
> +	if ((import.addr & 0xC0000000) != 0xC0000000) {
> +		pr_err("%s: Expecting an uncached alias for dma_addr %pad\n",
> +		       __func__, &dma_addr);
> +		import.addr |= 0xC0000000;
> +	}

Just so we don't forget about it, this shouldn't be needed once dma-ranges are
fixed.

> +	import.size = sg_dma_len(sgt->sgl);
> +	import.allocator = current->tgid;
> +	import.kernel_id = get_kernel_id(buffer);
> +
> +	memcpy(import.name, VC_SM_RESOURCE_NAME_DEFAULT,
> +	       sizeof(VC_SM_RESOURCE_NAME_DEFAULT));
> +
> +	pr_debug("[%s]: attempt to import \"%s\" data - type %u, addr %pad, size
> %u.\n",
> +		 __func__, import.name, import.type, &dma_addr, import.size);
> +
> +	/* Allocate the videocore buffer. */
> +	status = vc_sm_cma_vchi_import(sm_state->sm_handle, &import, &result,
> +				       &sm_state->int_trans_id);
> +	if (status == -EINTR) {
> +		pr_debug("[%s]: requesting import memory action restart
> (trans_id: %u)\n",
> +			 __func__, sm_state->int_trans_id);
> +		ret = -ERESTARTSYS;
> +		private->restart_sys = -EINTR;
> +		private->int_action = VC_SM_MSG_TYPE_IMPORT;
> +		goto error;
> +	} else if (status || !result.res_handle) {
> +		pr_debug("[%s]: failed to import memory on videocore (status:
> %u, trans_id: %u)\n",
> +			 __func__, status, sm_state->int_trans_id);
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	mutex_init(&buffer->lock);
> +	INIT_LIST_HEAD(&buffer->attachments);
> +	memcpy(buffer->name, import.name,
> +	       min(sizeof(buffer->name), sizeof(import.name) - 1));
> +
> +	/* Keep track of the buffer we created. */
> +	buffer->private = private;
> +	buffer->vc_handle = result.res_handle;
> +	buffer->size = import.size;
> +	buffer->vpu_state = VPU_MAPPED;
> +
> +	buffer->imported = 1;
> +	buffer->import.dma_buf = dma_buf;
> +
> +	buffer->import.attach = attach;
> +	buffer->import.sgt = sgt;
> +	buffer->dma_addr = dma_addr;
> +	buffer->in_use = 1;
> +	buffer->kernel_id = import.kernel_id;
> +
> +	/*
> +	 * We're done - we need to export a new dmabuf chaining through most
> +	 * functions, but enabling us to release our own internal references
> +	 * here.
> +	 */
> +	exp_info.ops = &dma_buf_import_ops;
> +	exp_info.size = import.size;
> +	exp_info.flags = O_RDWR;
> +	exp_info.priv = buffer;
> +
> +	buffer->dma_buf = dma_buf_export(&exp_info);

Could you comment on the need for this second dma_buf? I've only reviewed code
related to mmal-vchiq imports, but it seems to me that it would be saner to do
the unmapping and unattaching explicitly as opposed to having this second
buffer refcount hit 0. Although, I can imagine this being needed for the
userspace interface.

When you talk about moving to dmabuf heaps, I've pictured a specific dmabuf
heap for vc4 that takes care of all the importing and unimporting (aside from
cma allocations). Am I right? If so, I'm pretty confident we can do away with
this.

Regards,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]