On 02/17/2018 01:54 AM, Laura Abbott wrote:
> On 02/16/2018 04:17 AM, Alexey Skidanov wrote:
>>
>>
>> On 02/16/2018 01:48 AM, Laura Abbott wrote:
>>> On 02/12/2018 02:33 PM, Alexey Skidanov wrote:
>>>> Current ion kernel mapping implementation uses vmap() to map previously
>>>> allocated buffers into kernel virtual address space.
>>>>
>>>> On 32-bit platforms, vmap() might fail on large enough buffers due
>>>> to the
>>>> limited available vmalloc space. dma_buf_kmap() should guarantee that
>>>> only one page is mapped at a time.
>>>>
>>>> To fix this, kmap()/kmap_atomic() is used to implement the appropriate
>>>> interfaces.
>>>>
>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@xxxxxxxxx>
>>>> ---
>>>> drivers/staging/android/ion/ion.c | 97
>>>> +++++++++++++++++++--------------------
>>>> drivers/staging/android/ion/ion.h | 1 -
>>>> 2 files changed, 48 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/android/ion/ion.c
>>>> b/drivers/staging/android/ion/ion.c
>>>> index 57e0d80..75830e3 100644
>>>> --- a/drivers/staging/android/ion/ion.c
>>>> +++ b/drivers/staging/android/ion/ion.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <linux/slab.h>
>>>> #include <linux/uaccess.h>
>>>> #include <linux/vmalloc.h>
>>>> +#include <linux/highmem.h>
>>>> #include "ion.h"
>>>> @@ -119,8 +120,6 @@ static struct ion_buffer
>>>> *ion_buffer_create(struct ion_heap *heap,
>>>> void ion_buffer_destroy(struct ion_buffer *buffer)
>>>> {
>>>> - if (WARN_ON(buffer->kmap_cnt > 0))
>>>> - buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
>>>> buffer->heap->ops->free(buffer);
>>>> kfree(buffer);
>>>> }
>>>> @@ -140,34 +139,6 @@ static void _ion_buffer_destroy(struct ion_buffer
>>>> *buffer)
>>>> ion_buffer_destroy(buffer);
>>>> }
>>>> -static void *ion_buffer_kmap_get(struct ion_buffer *buffer)
>>>> -{
>>>> - void *vaddr;
>>>> -
>>>> - if (buffer->kmap_cnt) {
>>>> - buffer->kmap_cnt++;
>>>> - return buffer->vaddr;
>>>> - }
>>>> - vaddr = buffer->heap->ops->map_kernel(buffer->heap, buffer);
>>>> - if (WARN_ONCE(!vaddr,
>>>> - "heap->ops->map_kernel should return ERR_PTR on error"))
>>>> - return ERR_PTR(-EINVAL);
>>>> - if (IS_ERR(vaddr))
>>>> - return vaddr;
>>>> - buffer->vaddr = vaddr;
>>>> - buffer->kmap_cnt++;
>>>> - return vaddr;
>>>> -}
>>>> -
>>>> -static void ion_buffer_kmap_put(struct ion_buffer *buffer)
>>>> -{
>>>> - buffer->kmap_cnt--;
>>>> - if (!buffer->kmap_cnt) {
>>>> - buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
>>>> - buffer->vaddr = NULL;
>>>> - }
>>>> -}
>>>> -
>>>> static struct sg_table *dup_sg_table(struct sg_table *table)
>>>> {
>>>> struct sg_table *new_table;
>>>> @@ -305,34 +276,68 @@ static void ion_dma_buf_release(struct dma_buf
>>>> *dmabuf)
>>>> _ion_buffer_destroy(buffer);
>>>> }
>>>> +static inline int sg_page_count(struct scatterlist *sg)
>>>> +{
>>>> + return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
>>>> +}
>>>> +
>>>> +static struct page *ion_dma_buf_get_page(struct sg_table *sg_table,
>>>> + unsigned long offset)
>>>> +{
>>>> + struct page *page;
>>>> + struct scatterlist *sg;
>>>> + int i;
>>>> + unsigned int nr_pages;
>>>> +
>>>> + nr_pages = 0;
>>>> + page = NULL;
>>>> + /* Find the page with specified offset */
>>>> + for_each_sg(sg_table->sgl, sg, sg_table->nents, i) {
>>>> + if (nr_pages + sg_page_count(sg) > offset) {
>>>> + page = nth_page(sg_page(sg), offset - nr_pages);
>>>> + break;
>>>> + }
>>>> +
>>>> + nr_pages += sg_page_count(sg);
>>>> + }
>>>> +
>>>> + return page;
>>>> +}
>>>> static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long
>>>> offset)
>>>> {
>>>> struct ion_buffer *buffer = dmabuf->priv;
>>>> - return buffer->vaddr + offset * PAGE_SIZE;
>>>> + return kmap(ion_dma_buf_get_page(buffer->sg_table, offset));
>>>> }
>>>
>>> This unfortunately doesn't work for uncached buffers. We need to create
>>> an uncached alias otherwise we break what little coherency that
>>> guarantees.
>>> I'm still convinced that we can't actually do the uncached buffers
>>> correctly and they should be removed...
>>>
>>> Thanks,
>>> Laura
>>>
>> I assume that your concern is possible cache coherency issue as result
>> of the memory access with the different cache attributes. Especially, if
>> these accesses are from different context.
>>
>> But dma-buf requires that the cpu access should be started with
>> dma_buf_start_cpu_access() and ended with dma_buf_end_cpu_access() (and
>> with appropriate ioctl calls from the user space) to ensure that there
>> is no IO coherency issues. That is, the appropriate cache lines are
>> invalidated before the access and are flushed after the access (in case
>> of read/write access).
>>
>> So, it seems, that in the end of each CPU access, the most update copy
>> of the buffer is in the RAM.
>>
>> Probably, I'm wrong but I don't see the issue.
>>
>
> Part of the issue is that uncached buffers are often used so we don't
> need to do any cache operations. We do the sync currently but there's
> interest in removing it for uncached buffers.
This is great. It might have some performance improvement. So, the
dma-buf will be aware of uncached buffers? I mean,
dma_buf_start_end_cpu_access() will be actually noop for uncached buffers?
>
> I also haven't looked at aliasing rules on x86 closely but at least
> on arm, coherency with different cache attributes is difficult to
> get right and has a big "not recommended" warning in the description.
> You may be right that the above sequence would work out with the
> cpu accessors but it would need some careful review. The trade off
> of "might run out of vmalloc space" vs. "cache alias nightmare" doesn't
> seem worth it.
Completely agree with you. So, I would still use kmap() for regular
buffers and vmap() mapping only *one* page for uncached ones.
>
> Thanks,
> Laura
>
> P.S. I think the request from Greg was to explicitly list who looked
> at the patches with the Reviewed-by tag.
No one has reviewed the patch in details like you. Most of the comments
were about the commit message and why do I care about 32-bit platforms
and so on ... I have nothing more to add about it :(
Thanks,
Alexey
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
