Hi Pawel,
On 11/16/2014 02:13 PM, Pawel Osciak wrote:
> On Mon, Nov 10, 2014 at 8:49 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Require that dma-sg also uses an allocation context. This is in preparation
>> for adding prepare/finish memops to sync the memory between DMA and CPU.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>
> [...]
>
>> @@ -166,6 +177,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
>> unsigned long size,
>> enum dma_data_direction dma_dir)
>> {
>> + struct vb2_dma_sg_conf *conf = alloc_ctx;
>> struct vb2_dma_sg_buf *buf;
>> unsigned long first, last;
>> int num_pages_from_user;
>> @@ -235,6 +247,8 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
>> buf->num_pages, buf->offset, size, 0))
>> goto userptr_fail_alloc_table_from_pages;
>>
>> + /* Prevent the device from being released while the buffer is used */
>> + buf->dev = get_device(conf->dev);
>
> I'm not sure if we should be managing this... As far as I understand
> the logic behind taking a ref in alloc, if we are the exporter, we may
> have to keep it in case we need to free the buffers after our device
> goes away. But for userptr, we only need this for syncs, and in that
> case it's triggered by our driver, so I think we don't have to worry
> about that. If we do though, then dma-contig should be doing this as
> well.
You are correct. I'll remove this for get/put_userptr.
>
>> return buf;
>>
>> userptr_fail_alloc_table_from_pages:
>> @@ -274,6 +288,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
>> }
>> kfree(buf->pages);
>> vb2_put_vma(buf->vma);
>> + put_device(buf->dev);
>> kfree(buf);
>> }
>>
>> @@ -356,6 +371,27 @@ const struct vb2_mem_ops vb2_dma_sg_memops = {
>> };
>> EXPORT_SYMBOL_GPL(vb2_dma_sg_memops);
>>
>> +void *vb2_dma_sg_init_ctx(struct device *dev)
>> +{
>> + struct vb2_dma_sg_conf *conf;
>> +
>> + conf = kzalloc(sizeof(*conf), GFP_KERNEL);
>> + if (!conf)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + conf->dev = dev;
>> +
>> + return conf;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_dma_sg_init_ctx);
>> +
>> +void vb2_dma_sg_cleanup_ctx(void *alloc_ctx)
>> +{
>> + if (!IS_ERR_OR_NULL(alloc_ctx))
>
> I would prefer not doing this, it's very weird and would really just
> be a programming bug.
Erm, I rather like it since it allows you to call cleanup_ctx even if init_ctx
failed. It's actually used like that in the solo driver. Basically for the same
reason that kfree can handle a NULL pointer. Although if I do it here, then it
should also be added to dma-contig.
Regards,
Hans
--
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
