On Thu, Dec 20, 2012 at 10:50 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Thu, Dec 20, 2012 at 7:14 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
>> All drivers which implement this need to have some sort of refcount to
>> allow concurrent vmap usage. Hence implement this in the dma-buf core.
>>
>> To protect against concurrent calls we need a lock, which potentially
>> causes new funny locking inversions. But this shouldn't be a problem
>> for exporters with statically allocated backing storage, and more
>> dynamic drivers have decent issues already anyway.
>>
>> Inspired by some refactoring patches from Aaron Plattner, who
>> implemented the same idea, but only for drm/prime drivers.
>>
>> v2: Check in dma_buf_release that no dangling vmaps are left.
>> Suggested by Aaron Plattner. We might want to do similar checks for
>> attachments, but that's for another patch. Also fix up ERR_PTR return
>> for vmap.
>>
>> v3: Check whether the passed-in vmap address matches with the cached
>> one for vunmap. Eventually we might want to remove that parameter -
>> compared to the kmap functions there's no need for the vaddr for
>> unmapping. Suggested by Chris Wilson.
>>
>> v4: Fix a brown-paper-bag bug spotted by Aaron Plattner.
>
> yeah, I think the locking does worry me a bit but hopefully should be
> able to do something better when reservations land
>
> Signed-off-by: Rob Clark <rob@xxxxxx>
or even,
Reviewed-by: Rob Clark <rob@xxxxxx>
>
>> Cc: Aaron Plattner <aplattner@xxxxxxxxxx>
>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> ---
>> Documentation/dma-buf-sharing.txt | 6 +++++-
>> drivers/base/dma-buf.c | 43 ++++++++++++++++++++++++++++++++++-----
>> include/linux/dma-buf.h | 4 +++-
>> 3 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
>> index 0188903..4966b1b 100644
>> --- a/Documentation/dma-buf-sharing.txt
>> +++ b/Documentation/dma-buf-sharing.txt
>> @@ -302,7 +302,11 @@ Access to a dma_buf from the kernel context involves three steps:
>> void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
>>
>> The vmap call can fail if there is no vmap support in the exporter, or if it
>> - runs out of vmalloc space. Fallback to kmap should be implemented.
>> + runs out of vmalloc space. Fallback to kmap should be implemented. Note that
>> + the dma-buf layer keeps a reference count for all vmap access and calls down
>> + into the exporter's vmap function only when no vmapping exists, and only
>> + unmaps it once. Protection against concurrent vmap/vunmap calls is provided
>> + by taking the dma_buf->lock mutex.
>>
>> 3. Finish access
>>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index a3f79c4..26b68de 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>> @@ -39,6 +39,8 @@ static int dma_buf_release(struct inode *inode, struct file *file)
>>
>> dmabuf = file->private_data;
>>
>> + BUG_ON(dmabuf->vmapping_counter);
>> +
>> dmabuf->ops->release(dmabuf);
>> kfree(dmabuf);
>> return 0;
>> @@ -482,12 +484,34 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
>> */
>> void *dma_buf_vmap(struct dma_buf *dmabuf)
>> {
>> + void *ptr;
>> +
>> if (WARN_ON(!dmabuf))
>> return NULL;
>>
>> - if (dmabuf->ops->vmap)
>> - return dmabuf->ops->vmap(dmabuf);
>> - return NULL;
>> + if (!dmabuf->ops->vmap)
>> + return NULL;
>> +
>> + mutex_lock(&dmabuf->lock);
>> + if (dmabuf->vmapping_counter) {
>> + dmabuf->vmapping_counter++;
>> + BUG_ON(!dmabuf->vmap_ptr);
>> + ptr = dmabuf->vmap_ptr;
>> + goto out_unlock;
>> + }
>> +
>> + BUG_ON(dmabuf->vmap_ptr);
>> +
>> + ptr = dmabuf->ops->vmap(dmabuf);
>> + if (IS_ERR_OR_NULL(ptr))
>> + goto out_unlock;
>> +
>> + dmabuf->vmap_ptr = ptr;
>> + dmabuf->vmapping_counter = 1;
>> +
>> +out_unlock:
>> + mutex_unlock(&dmabuf->lock);
>> + return ptr;
>> }
>> EXPORT_SYMBOL_GPL(dma_buf_vmap);
>>
>> @@ -501,7 +525,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
>> if (WARN_ON(!dmabuf))
>> return;
>>
>> - if (dmabuf->ops->vunmap)
>> - dmabuf->ops->vunmap(dmabuf, vaddr);
>> + BUG_ON(!dmabuf->vmap_ptr);
>> + BUG_ON(dmabuf->vmapping_counter == 0);
>> + BUG_ON(dmabuf->vmap_ptr != vaddr);
>> +
>> + mutex_lock(&dmabuf->lock);
>> + if (--dmabuf->vmapping_counter == 0) {
>> + if (dmabuf->ops->vunmap)
>> + dmabuf->ops->vunmap(dmabuf, vaddr);
>> + dmabuf->vmap_ptr = NULL;
>> + }
>> + mutex_unlock(&dmabuf->lock);
>> }
>> EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index bd2e52c..e3bf2f6 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -119,8 +119,10 @@ struct dma_buf {
>> struct file *file;
>> struct list_head attachments;
>> const struct dma_buf_ops *ops;
>> - /* mutex to serialize list manipulation and attach/detach */
>> + /* mutex to serialize list manipulation, attach/detach and vmap/unmap */
>> struct mutex lock;
>> + unsigned vmapping_counter;
>> + void *vmap_ptr;
>> void *priv;
>> };
>>
>> --
>> 1.7.11.7
>>
>> --
>> 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
--
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
