Re: [PATCH v7 2/3] SMAF: add CMA allocator

[Benjamin Gaignard]

Hi Emil,

2016-05-17 1:05 GMT+02:00 Emil Velikov <emil.l.velikov@xxxxxxxxx>:
> 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 ?
>

Device dma_bit_mask field has to be set to DMA_BIT_MASK(32) to target
a CMA area.
I haven't see any other #define for 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.

Here I have duplicate what is done everywhere else but I could try to
add it into
smaf_cma_buffer_info structure.

>
>> +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 ?
>

I haven't used DMA_ATTR_NO_KERNEL_MAPPING while allocating the buffer
so kernel mapping is set by default and I don't have to manage map refcounting.

>> +}
>> +
>> +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' ?

sure
>
>
>> +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.
>

If the returned device have an associated CMA area then it will use it else
if dev have not CMA area or if find_matching_device() return NULL
the default CMA area will be used

>> +       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' ?
>
I will remove it

>> +               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.
>
OK

>> +               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 ?
>

Yes for sure

>
> Regards,
> Emil



-- 
Benjamin Gaignard

Graphic Working Group

Linaro.org â Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog