Hi Dave,
some questions/comments.
On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> ---
[...]
> +
> +/* 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:
signature.asc
Description: This is a digitally signed message part
