Hi Benjamin,
On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx> wrote:
> SMAF CMA allocator implement helpers functions to allow SMAF
> to allocate contiguous memory.
>
> match() each if at least one of the attached devices have coherent_dma_mask
> set to DMA_BIT_MASK(32).
>
What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?
> +static void smaf_cma_release(struct dma_buf *dmabuf)
> +{
> + struct smaf_cma_buffer_info *info = dmabuf->priv;
> + DEFINE_DMA_ATTRS(attrs);
> +
> + dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
This way it's less likely for things to go wrong, if one forgets to
update one of the three in the future.
> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
> + struct sg_table *sgt,
> + enum dma_data_direction direction)
> +{
> + /* do nothing */
There could/should really be a comment explaining why we "do nothing"
here, right ?
> +}
> +
> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> + struct smaf_cma_buffer_info *info = dmabuf->priv;
> + int ret;
> + DEFINE_DMA_ATTRS(attrs);
> +
> + dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> + if (info->size < vma->vm_end - vma->vm_start)
> + return -EINVAL;
> +
> + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> + ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
> + info->size, &attrs);
> +
> + return ret;
Kill the temporary variable 'ret' ?
> +static struct dma_buf_ops smaf_cma_ops = {
const ? Afaict the compiler would/should warn you about discarding it
as the ops are defined const.
> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
> + size_t length, unsigned int flags)
> +{
> + struct dma_buf_attachment *attach_obj;
> + struct smaf_cma_buffer_info *info;
> + struct dma_buf *cma_dmabuf;
> + int ret;
> +
> + DEFINE_DMA_BUF_EXPORT_INFO(export);
> + DEFINE_DMA_ATTRS(attrs);
> +
> + dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return NULL;
> +
> + info->dev = find_matching_device(dmabuf);
find_matching_device() can return NULL. We should handle that imho.
> + info->size = length;
> + info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
> + GFP_KERNEL | __GFP_NOWARN, &attrs);
> + if (!info->vaddr) {
> + ret = -ENOMEM;
set-but-unused-variable 'ret' ?
> + goto error;
> + }
> +
> + export.ops = &smaf_cma_ops;
> + export.size = info->size;
> + export.flags = flags;
> + export.priv = info;
> +
> + cma_dmabuf = dma_buf_export(&export);
> + if (IS_ERR(cma_dmabuf))
Missing dma_free_attrs() ? I'd add another label in the error path and
handle it there.
> + goto error;
> +
> + list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
> + dma_buf_attach(cma_dmabuf, attach_obj->dev);
Imho one should error out if attach fails. Or warn at the very least ?
> +static int __init smaf_cma_init(void)
> +{
> + INIT_LIST_HEAD(&smaf_cma.list_node);
Isn't this something that smaf_register_allocator() should be doing ?
Regards,
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
