[PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

Any driver may access shared buffers, created by ion, using dma_buf_vmap and
dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
the kernel virtual address space. The implementation of these API is missing in
the current ion implementation.

Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
---
 drivers/staging/android/ion/ion.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index f480885..4f1dc7f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,
 {
 }
 
+static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
+{
+	struct ion_buffer *buffer = dmabuf->priv;
+
+	return buffer->vaddr;
+}
+
+static void ion_dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
+{
+}
+
 static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
 					enum dma_data_direction direction)
 {
@@ -388,6 +399,8 @@ static const struct dma_buf_ops dma_buf_ops = {
 	.unmap_atomic = ion_dma_buf_kunmap,
 	.map = ion_dma_buf_kmap,
 	.unmap = ion_dma_buf_kunmap,
+	.vmap = ion_dma_buf_vmap,
+	.vunmap = ion_dma_buf_vunmap
 };
 
 int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Greg KH]

On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
> Any driver may access shared buffers, created by ion, using dma_buf_vmap and
> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
> the kernel virtual address space. The implementation of these API is missing in
> the current ion implementation.
> 
> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
> ---

No review from any other Intel developers? :(

Anyway, what in-tree driver needs access to these functions?

And are you sure that you don't need to do any "real" logic in the
vmap/vunmap calls?  That feels like there would be some reference
counting problems here.

>  drivers/staging/android/ion/ion.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index f480885..4f1dc7f 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,
>  {
>  }
>  
> +static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
> +{
> +	struct ion_buffer *buffer = dmabuf->priv;
> +
> +	return buffer->vaddr;

Just call ion_dma_buf_kmap(dmabuf, 0)?

Again, please get this reviewed by someone else in Intel first.  Don't
ignore the resources you have, to do so would be foolish :)

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 01/31/2018 03:00 PM, Greg KH wrote:
> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>> Any driver may access shared buffers, created by ion, using dma_buf_vmap and
>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
>> the kernel virtual address space. The implementation of these API is missing in
>> the current ion implementation.
>>
>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
>> ---
> 
> No review from any other Intel developers? :(
Will add.
> 
> Anyway, what in-tree driver needs access to these functions?
I'm not sure that there are the in-tree drivers using these functions 
and ion as buffer exporter because they are not implemented in ion :) 
But there are some in-tree drivers using these APIs (gpu drivers) with 
other buffer exporters.
> 
> And are you sure that you don't need to do any "real" logic in the
> vmap/vunmap calls?  That feels like there would be some reference
> counting problems here.
dma_buf_start_cpu_access is called before the call to dma_buf_vmap. It 
actually increments the reference count. dma_buf_end_cpu_access is 
called after the dma_buf_vunmap. It actually decrements the reference 
count.>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index f480885..4f1dc7f 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -327,6 +327,17 @@ static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset,
>>   {
>>   }
>>   
>> +static void *ion_dma_buf_vmap(struct dma_buf *dmabuf)
>> +{
>> +	struct ion_buffer *buffer = dmabuf->priv;
>> +
>> +	return buffer->vaddr;
> 
> Just call ion_dma_buf_kmap(dmabuf, 0)?
Sure.
> 
> Again, please get this reviewed by someone else in Intel first.  Don't
> ignore the resources you have, to do so would be foolish :)
Sure. Will do.
> 
> thanks,
> 
> greg k-h
> 

Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Laura Abbott]

On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
> 
> On 01/31/2018 03:00 PM, Greg KH wrote:
>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>> Any driver may access shared buffers, created by ion, using dma_buf_vmap and
>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated buffers into
>>> the kernel virtual address space. The implementation of these API is missing in
>>> the current ion implementation.
>>>
>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
>>> ---
>>
>> No review from any other Intel developers? :(
> Will add.
>>
>> Anyway, what in-tree driver needs access to these functions?
> I'm not sure that there are the in-tree drivers using these functions and ion as> buffer exporter because they are not implemented in ion :) But there are some in-tre> drivers using these APIs (gpu drivers) with other buffer exporters.

It's still not clear why you need to implement these APIs.
Are you planning to use Ion with GPU drivers? I'm especially
interested in this if you have a non-Android use case.

Thanks,
Laura
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 02/07/2018 01:56 AM, Laura Abbott wrote:
> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>
>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>> Any driver may access shared buffers, created by ion, using
>>>> dma_buf_vmap and
>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>> buffers into
>>>> the kernel virtual address space. The implementation of these API is
>>>> missing in
>>>> the current ion implementation.
>>>>
>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
>>>> ---
>>>
>>> No review from any other Intel developers? :(
>> Will add.
>>>
>>> Anyway, what in-tree driver needs access to these functions?
>> I'm not sure that there are the in-tree drivers using these functions
>> and ion as> buffer exporter because they are not implemented in ion :)
>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>> other buffer exporters.
> 
> It's still not clear why you need to implement these APIs.
How the importing kernel module may access the content of the buffer? :)
With the current ion implementation it's only possible by dma_buf_kmap,
mapping one page at a time. For pretty large buffers, it might have some
performance impact.
(Probably, the page by page mapping is the only way to access large
buffers on 32 bit systems, where the vmalloc range is very small. By the
way, the current ion dma_map_kmap doesn't really map only 1 page at a
time - it uses the result of vmap() that might fail on 32 bit systems.)

> Are you planning to use Ion with GPU drivers? I'm especially
> interested in this if you have a non-Android use case.
Yes, my use case is the non-Android one. But not with GPU drivers.
> 
> Thanks,
> Laura

Thanks,
Alexey
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Liam Mark]

On Tue, 6 Feb 2018, Alexey Skidanov wrote:

> 
> 
> On 02/07/2018 01:56 AM, Laura Abbott wrote:
> > On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
> >>
> >> On 01/31/2018 03:00 PM, Greg KH wrote:
> >>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
> >>>> Any driver may access shared buffers, created by ion, using
> >>>> dma_buf_vmap and
> >>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
> >>>> buffers into
> >>>> the kernel virtual address space. The implementation of these API is
> >>>> missing in
> >>>> the current ion implementation.
> >>>>
> >>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
> >>>> ---
> >>>
> >>> No review from any other Intel developers? :(
> >> Will add.
> >>>
> >>> Anyway, what in-tree driver needs access to these functions?
> >> I'm not sure that there are the in-tree drivers using these functions
> >> and ion as> buffer exporter because they are not implemented in ion :)
> >> But there are some in-tre> drivers using these APIs (gpu drivers) with
> >> other buffer exporters.
> > 
> > It's still not clear why you need to implement these APIs.
> How the importing kernel module may access the content of the buffer? :)
> With the current ion implementation it's only possible by dma_buf_kmap,
> mapping one page at a time. For pretty large buffers, it might have some
> performance impact.
> (Probably, the page by page mapping is the only way to access large
> buffers on 32 bit systems, where the vmalloc range is very small. By the
> way, the current ion dma_map_kmap doesn't really map only 1 page at a
> time - it uses the result of vmap() that might fail on 32 bit systems.)
> 
> > Are you planning to use Ion with GPU drivers? I'm especially
> > interested in this if you have a non-Android use case.
> Yes, my use case is the non-Android one. But not with GPU drivers.
> > 
> > Thanks,
> > Laura
> 
> Thanks,
> Alexey

I was wondering if we could re-open the discussion on adding support to 
ION for dma_buf_vmap.
It seems like the patch was not taken as the reviewers wanted more 
evidence of an upstream use case.

Here would be my upstream usage argument for including dma_buf_vmap 
support in ION.

Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
of a kernel mapping for the buffer, unfortunately the resulting call to 
alloc_vmap_area can be quite expensive and this has caused a performance 
regression for certain clients when they have moved to the new version of 
ION.

The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
and generally isn't needed by clients. So if we remove the creation of the 
kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
needed we can speed up the calls to ion_dma_buf_begin_cpu_access.

An additional benefit of removing the creation of kernel mappings from 
ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
go negative which could lead to undesired behavior.

One disadvantage of the above change is that a kernel mapping is not 
already created when a client calls dma_buf_kmap. So the following 
dma_buf_kmap contract can't be satisfied.

/**
* dma_buf_kmap - Map a page of the buffer object into kernel address 
space. The
* same restrictions as for kmap and friends apply.
* @dmabuf:	[in]	buffer to map page from.
* @page_num:	[in]	page in PAGE_SIZE units to map.
*
* This call must always succeed, any necessary preparations that might 
fail
* need to be done in begin_cpu_access.
*/

But hopefully we can work around this by moving clients to dma_buf_vmap.

Based on discussions at LPC here is what was proposed:
- #1 Add support to ION for dma_buf_vmap and dma_buf_vunmap
- #2 Move any existing ION clients over from using dma_buf_kmap to 
dma_buf_vmap
- #3 Deprecate support in ION for dma_buf_kmap?
- #4 Make the above performance optimization to 
ion_dma_buf_begin_cpu_access to remove the creation of a kernel mapping.

Thoughts?

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 12/16/18 7:20 AM, Liam Mark wrote:
> On Tue, 6 Feb 2018, Alexey Skidanov wrote:
> 
>>
>>
>> On 02/07/2018 01:56 AM, Laura Abbott wrote:
>>> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>>>
>>>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>>>> Any driver may access shared buffers, created by ion, using
>>>>>> dma_buf_vmap and
>>>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>>>> buffers into
>>>>>> the kernel virtual address space. The implementation of these API is
>>>>>> missing in
>>>>>> the current ion implementation.
>>>>>>
>>>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
>>>>>> ---
>>>>>
>>>>> No review from any other Intel developers? :(
>>>> Will add.
>>>>>
>>>>> Anyway, what in-tree driver needs access to these functions?
>>>> I'm not sure that there are the in-tree drivers using these functions
>>>> and ion as> buffer exporter because they are not implemented in ion :)
>>>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>>>> other buffer exporters.
>>>
>>> It's still not clear why you need to implement these APIs.
>> How the importing kernel module may access the content of the buffer? :)
>> With the current ion implementation it's only possible by dma_buf_kmap,
>> mapping one page at a time. For pretty large buffers, it might have some
>> performance impact.
>> (Probably, the page by page mapping is the only way to access large
>> buffers on 32 bit systems, where the vmalloc range is very small. By the
>> way, the current ion dma_map_kmap doesn't really map only 1 page at a
>> time - it uses the result of vmap() that might fail on 32 bit systems.)
>>
>>> Are you planning to use Ion with GPU drivers? I'm especially
>>> interested in this if you have a non-Android use case.
>> Yes, my use case is the non-Android one. But not with GPU drivers.
>>>
>>> Thanks,
>>> Laura
>>
>> Thanks,
>> Alexey
> 
> I was wondering if we could re-open the discussion on adding support to 
> ION for dma_buf_vmap.
> It seems like the patch was not taken as the reviewers wanted more 
> evidence of an upstream use case.
> 
> Here would be my upstream usage argument for including dma_buf_vmap 
> support in ION.
> 
> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
> of a kernel mapping for the buffer, unfortunately the resulting call to 
> alloc_vmap_area can be quite expensive and this has caused a performance 
> regression for certain clients when they have moved to the new version of 
> ION.
> 
> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
> and generally isn't needed by clients. So if we remove the creation of the 
> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> 
> An additional benefit of removing the creation of kernel mappings from 
> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
> go negative which could lead to undesired behavior.
> 
> One disadvantage of the above change is that a kernel mapping is not 
> already created when a client calls dma_buf_kmap. So the following 
> dma_buf_kmap contract can't be satisfied.
> 
> /**
> * dma_buf_kmap - Map a page of the buffer object into kernel address 
> space. The
> * same restrictions as for kmap and friends apply.
> * @dmabuf:	[in]	buffer to map page from.
> * @page_num:	[in]	page in PAGE_SIZE units to map.
> *
> * This call must always succeed, any necessary preparations that might 
> fail
> * need to be done in begin_cpu_access.
> */
> 
> But hopefully we can work around this by moving clients to dma_buf_vmap.
I think the problem is with the contract. We can't ensure that the call
is always succeeds regardless the implementation - any mapping might
fail. Probably this is why  *all* clients of dma_buf_kmap() check the
return value (so it's safe to return NULL in case of failure).

I would suggest to fix the contract and to keep the dma_buf_kmap()
support in ION.
> 
> Based on discussions at LPC here is what was proposed:
> - #1 Add support to ION for dma_buf_vmap and dma_buf_vunmap
> - #2 Move any existing ION clients over from using dma_buf_kmap to 
> dma_buf_vmap
> - #3 Deprecate support in ION for dma_buf_kmap?
> - #4 Make the above performance optimization to 
> ion_dma_buf_begin_cpu_access to remove the creation of a kernel mapping.
> 
> Thoughts?
> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Thanks,
Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Liam Mark]

On Sun, 16 Dec 2018, Alexey Skidanov wrote:

> 
> 
> On 12/16/18 7:20 AM, Liam Mark wrote:
> > On Tue, 6 Feb 2018, Alexey Skidanov wrote:
> > 
> >>
> >>
> >> On 02/07/2018 01:56 AM, Laura Abbott wrote:
> >>> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
> >>>>
> >>>> On 01/31/2018 03:00 PM, Greg KH wrote:
> >>>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
> >>>>>> Any driver may access shared buffers, created by ion, using
> >>>>>> dma_buf_vmap and
> >>>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
> >>>>>> buffers into
> >>>>>> the kernel virtual address space. The implementation of these API is
> >>>>>> missing in
> >>>>>> the current ion implementation.
> >>>>>>
> >>>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
> >>>>>> ---
> >>>>>
> >>>>> No review from any other Intel developers? :(
> >>>> Will add.
> >>>>>
> >>>>> Anyway, what in-tree driver needs access to these functions?
> >>>> I'm not sure that there are the in-tree drivers using these functions
> >>>> and ion as> buffer exporter because they are not implemented in ion :)
> >>>> But there are some in-tre> drivers using these APIs (gpu drivers) with
> >>>> other buffer exporters.
> >>>
> >>> It's still not clear why you need to implement these APIs.
> >> How the importing kernel module may access the content of the buffer? :)
> >> With the current ion implementation it's only possible by dma_buf_kmap,
> >> mapping one page at a time. For pretty large buffers, it might have some
> >> performance impact.
> >> (Probably, the page by page mapping is the only way to access large
> >> buffers on 32 bit systems, where the vmalloc range is very small. By the
> >> way, the current ion dma_map_kmap doesn't really map only 1 page at a
> >> time - it uses the result of vmap() that might fail on 32 bit systems.)
> >>
> >>> Are you planning to use Ion with GPU drivers? I'm especially
> >>> interested in this if you have a non-Android use case.
> >> Yes, my use case is the non-Android one. But not with GPU drivers.
> >>>
> >>> Thanks,
> >>> Laura
> >>
> >> Thanks,
> >> Alexey
> > 
> > I was wondering if we could re-open the discussion on adding support to 
> > ION for dma_buf_vmap.
> > It seems like the patch was not taken as the reviewers wanted more 
> > evidence of an upstream use case.
> > 
> > Here would be my upstream usage argument for including dma_buf_vmap 
> > support in ION.
> > 
> > Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
> > of a kernel mapping for the buffer, unfortunately the resulting call to 
> > alloc_vmap_area can be quite expensive and this has caused a performance 
> > regression for certain clients when they have moved to the new version of 
> > ION.
> > 
> > The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
> > and generally isn't needed by clients. So if we remove the creation of the 
> > kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
> > needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> > 
> > An additional benefit of removing the creation of kernel mappings from 
> > ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> > Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
> > flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
> > go negative which could lead to undesired behavior.
> > 
> > One disadvantage of the above change is that a kernel mapping is not 
> > already created when a client calls dma_buf_kmap. So the following 
> > dma_buf_kmap contract can't be satisfied.
> > 
> > /**
> > * dma_buf_kmap - Map a page of the buffer object into kernel address 
> > space. The
> > * same restrictions as for kmap and friends apply.
> > * @dmabuf:	[in]	buffer to map page from.
> > * @page_num:	[in]	page in PAGE_SIZE units to map.
> > *
> > * This call must always succeed, any necessary preparations that might 
> > fail
> > * need to be done in begin_cpu_access.
> > */
> > 
> > But hopefully we can work around this by moving clients to dma_buf_vmap.
> I think the problem is with the contract. We can't ensure that the call
> is always succeeds regardless the implementation - any mapping might
> fail. Probably this is why  *all* clients of dma_buf_kmap() check the
> return value (so it's safe to return NULL in case of failure).
> 

I think currently the call to dma_buf_kmap will always succeed since the 
DMA-Buf contract requires that the client first successfully call 
dma_buf_begin_cpu_access(), and if dma_buf_begin_cpu_access() succeeds 
then dma_buf_kmap will succeed.

> I would suggest to fix the contract and to keep the dma_buf_kmap()
> support in ION.

I will leave it to the DMA-Buf maintainers as to whether they want to 
change their contract.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Alexey Skidanov]

On 12/17/18 20:42, Liam Mark wrote:
> On Sun, 16 Dec 2018, Alexey Skidanov wrote:
> 
>>
>>
>> On 12/16/18 7:20 AM, Liam Mark wrote:
>>> On Tue, 6 Feb 2018, Alexey Skidanov wrote:
>>>
>>>>
>>>>
>>>> On 02/07/2018 01:56 AM, Laura Abbott wrote:
>>>>> On 01/31/2018 10:10 PM, Alexey Skidanov wrote:
>>>>>>
>>>>>> On 01/31/2018 03:00 PM, Greg KH wrote:
>>>>>>> On Wed, Jan 31, 2018 at 02:03:42PM +0200, Alexey Skidanov wrote:
>>>>>>>> Any driver may access shared buffers, created by ion, using
>>>>>>>> dma_buf_vmap and
>>>>>>>> dma_buf_vunmap dma-buf API that maps/unmaps previosuly allocated
>>>>>>>> buffers into
>>>>>>>> the kernel virtual address space. The implementation of these API is
>>>>>>>> missing in
>>>>>>>> the current ion implementation.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@intel.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> No review from any other Intel developers? :(
>>>>>> Will add.
>>>>>>>
>>>>>>> Anyway, what in-tree driver needs access to these functions?
>>>>>> I'm not sure that there are the in-tree drivers using these functions
>>>>>> and ion as> buffer exporter because they are not implemented in ion :)
>>>>>> But there are some in-tre> drivers using these APIs (gpu drivers) with
>>>>>> other buffer exporters.
>>>>>
>>>>> It's still not clear why you need to implement these APIs.
>>>> How the importing kernel module may access the content of the buffer? :)
>>>> With the current ion implementation it's only possible by dma_buf_kmap,
>>>> mapping one page at a time. For pretty large buffers, it might have some
>>>> performance impact.
>>>> (Probably, the page by page mapping is the only way to access large
>>>> buffers on 32 bit systems, where the vmalloc range is very small. By the
>>>> way, the current ion dma_map_kmap doesn't really map only 1 page at a
>>>> time - it uses the result of vmap() that might fail on 32 bit systems.)
>>>>
>>>>> Are you planning to use Ion with GPU drivers? I'm especially
>>>>> interested in this if you have a non-Android use case.
>>>> Yes, my use case is the non-Android one. But not with GPU drivers.
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>>
>>>> Thanks,
>>>> Alexey
>>>
>>> I was wondering if we could re-open the discussion on adding support to 
>>> ION for dma_buf_vmap.
>>> It seems like the patch was not taken as the reviewers wanted more 
>>> evidence of an upstream use case.
>>>
>>> Here would be my upstream usage argument for including dma_buf_vmap 
>>> support in ION.
>>>
>>> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
>>> of a kernel mapping for the buffer, unfortunately the resulting call to 
>>> alloc_vmap_area can be quite expensive and this has caused a performance 
>>> regression for certain clients when they have moved to the new version of 
>>> ION.
>>>
>>> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
>>> and generally isn't needed by clients. So if we remove the creation of the 
>>> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
>>> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
>>>
>>> An additional benefit of removing the creation of kernel mappings from 
>>> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
>>> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
>>> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
>>> go negative which could lead to undesired behavior.
>>>
>>> One disadvantage of the above change is that a kernel mapping is not 
>>> already created when a client calls dma_buf_kmap. So the following 
>>> dma_buf_kmap contract can't be satisfied.
>>>
>>> /**
>>> * dma_buf_kmap - Map a page of the buffer object into kernel address 
>>> space. The
>>> * same restrictions as for kmap and friends apply.
>>> * @dmabuf:	[in]	buffer to map page from.
>>> * @page_num:	[in]	page in PAGE_SIZE units to map.
>>> *
>>> * This call must always succeed, any necessary preparations that might 
>>> fail
>>> * need to be done in begin_cpu_access.
>>> */
>>>
>>> But hopefully we can work around this by moving clients to dma_buf_vmap.
>> I think the problem is with the contract. We can't ensure that the call
>> is always succeeds regardless the implementation - any mapping might
>> fail. Probably this is why  *all* clients of dma_buf_kmap() check the
>> return value (so it's safe to return NULL in case of failure).
>>
> 
> I think currently the call to dma_buf_kmap will always succeed since the 
> DMA-Buf contract requires that the client first successfully call 
> dma_buf_begin_cpu_access(), and if dma_buf_begin_cpu_access() succeeds 
> then dma_buf_kmap will succeed.
> 
>> I would suggest to fix the contract and to keep the dma_buf_kmap()
>> support in ION.
> 
> I will leave it to the DMA-Buf maintainers as to whether they want to 
> change their contract.
> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Ok. We need the list of the clients using the ION in the mainline tree.

Alexey

Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Liam Mark]

On Tue, 18 Dec 2018, Alexey Skidanov wrote:

> >>> I was wondering if we could re-open the discussion on adding support to 
> >>> ION for dma_buf_vmap.
> >>> It seems like the patch was not taken as the reviewers wanted more 
> >>> evidence of an upstream use case.
> >>>
> >>> Here would be my upstream usage argument for including dma_buf_vmap 
> >>> support in ION.
> >>>
> >>> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation 
> >>> of a kernel mapping for the buffer, unfortunately the resulting call to 
> >>> alloc_vmap_area can be quite expensive and this has caused a performance 
> >>> regression for certain clients when they have moved to the new version of 
> >>> ION.
> >>>
> >>> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access, 
> >>> and generally isn't needed by clients. So if we remove the creation of the 
> >>> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when 
> >>> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> >>>
> >>> An additional benefit of removing the creation of kernel mappings from 
> >>> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> >>> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with 
> >>> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to 
> >>> go negative which could lead to undesired behavior.
> >>>
> >>> One disadvantage of the above change is that a kernel mapping is not 
> >>> already created when a client calls dma_buf_kmap. So the following 
> >>> dma_buf_kmap contract can't be satisfied.
> >>>
> >>> /**
> >>> * dma_buf_kmap - Map a page of the buffer object into kernel address 
> >>> space. The
> >>> * same restrictions as for kmap and friends apply.
> >>> * @dmabuf:	[in]	buffer to map page from.
> >>> * @page_num:	[in]	page in PAGE_SIZE units to map.
> >>> *
> >>> * This call must always succeed, any necessary preparations that might 
> >>> fail
> >>> * need to be done in begin_cpu_access.
> >>> */
> >>>
> >>> But hopefully we can work around this by moving clients to dma_buf_vmap.
> >> I think the problem is with the contract. We can't ensure that the call
> >> is always succeeds regardless the implementation - any mapping might
> >> fail. Probably this is why  *all* clients of dma_buf_kmap() check the
> >> return value (so it's safe to return NULL in case of failure).
> >>
> > 
> > I think currently the call to dma_buf_kmap will always succeed since the 
> > DMA-Buf contract requires that the client first successfully call 
> > dma_buf_begin_cpu_access(), and if dma_buf_begin_cpu_access() succeeds 
> > then dma_buf_kmap will succeed.
> > 
> >> I would suggest to fix the contract and to keep the dma_buf_kmap()
> >> support in ION.
> > 
> > I will leave it to the DMA-Buf maintainers as to whether they want to 
> > change their contract.
> > 
> > Liam
> > 
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 
> 
> Ok. We need the list of the clients using the ION in the mainline tree.
> 

Looks to me like the only functions which might be calling 
dma_buf_kmap/dma_buf_kunmap on ION buffers are 
tegra_bo_kmap/tegra_bo_kunmap, I assume Tegra is used in some Android 
automotive products.

Looks like these functions could be moved over to using 
dma_buf_vmap/dma_buf_vunmap but it wouldn't be very clean and would add a 
performance hit.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Skidanov, Alexey]

> -----Original Message-----
> From: Liam Mark [mailto:lmark@codeaurora.org]
> Sent: Friday, January 04, 2019 19:42
> To: Skidanov, Alexey <alexey.skidanov@intel.com>
> Cc: Laura Abbott <labbott@redhat.com>; Greg KH <gregkh@linuxfoundation.org>;
> devel@driverdev.osuosl.org; tkjos@android.com; rve@android.com; linux-
> kernel@vger.kernel.org; maco@android.com; sumit.semwal@linaro.org
> Subject: Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and
> dma_buf_vunmap
> 
> On Tue, 18 Dec 2018, Alexey Skidanov wrote:
> 
> > >>> I was wondering if we could re-open the discussion on adding support to
> > >>> ION for dma_buf_vmap.
> > >>> It seems like the patch was not taken as the reviewers wanted more
> > >>> evidence of an upstream use case.
> > >>>
> > >>> Here would be my upstream usage argument for including dma_buf_vmap
> > >>> support in ION.
> > >>>
> > >>> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation
> > >>> of a kernel mapping for the buffer, unfortunately the resulting call to
> > >>> alloc_vmap_area can be quite expensive and this has caused a performance
> > >>> regression for certain clients when they have moved to the new version of
> > >>> ION.
> > >>>
> > >>> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access,
> > >>> and generally isn't needed by clients. So if we remove the creation of the
> > >>> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when
> > >>> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> > >>>
> > >>> An additional benefit of removing the creation of kernel mappings from
> > >>> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> > >>> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with
> > >>> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to
> > >>> go negative which could lead to undesired behavior.
> > >>>
> > >>> One disadvantage of the above change is that a kernel mapping is not
> > >>> already created when a client calls dma_buf_kmap. So the following
> > >>> dma_buf_kmap contract can't be satisfied.
> > >>>
> > >>> /**
> > >>> * dma_buf_kmap - Map a page of the buffer object into kernel address
> > >>> space. The
> > >>> * same restrictions as for kmap and friends apply.
> > >>> * @dmabuf:	[in]	buffer to map page from.
> > >>> * @page_num:	[in]	page in PAGE_SIZE units to map.
> > >>> *
> > >>> * This call must always succeed, any necessary preparations that might
> > >>> fail
> > >>> * need to be done in begin_cpu_access.
> > >>> */
> > >>>
> > >>> But hopefully we can work around this by moving clients to dma_buf_vmap.
> > >> I think the problem is with the contract. We can't ensure that the call
> > >> is always succeeds regardless the implementation - any mapping might
> > >> fail. Probably this is why  *all* clients of dma_buf_kmap() check the
> > >> return value (so it's safe to return NULL in case of failure).
> > >>
> > >
> > > I think currently the call to dma_buf_kmap will always succeed since the
> > > DMA-Buf contract requires that the client first successfully call
> > > dma_buf_begin_cpu_access(), and if dma_buf_begin_cpu_access() succeeds
> > > then dma_buf_kmap will succeed.
> > >
> > >> I would suggest to fix the contract and to keep the dma_buf_kmap()
> > >> support in ION.
> > >
> > > I will leave it to the DMA-Buf maintainers as to whether they want to
> > > change their contract.
> > >
> > > Liam
> > >
> > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > >
> >
> > Ok. We need the list of the clients using the ION in the mainline tree.
> >
> 
> Looks to me like the only functions which might be calling
> dma_buf_kmap/dma_buf_kunmap on ION buffers are
> tegra_bo_kmap/tegra_bo_kunmap, I assume Tegra is used in some Android
> automotive products.
> 
> Looks like these functions could be moved over to using
> dma_buf_vmap/dma_buf_vunmap but it wouldn't be very clean and would add a
> performance hit.
> 
> Liam
> 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

I'm a little bit confused. Why making the buffer accessible by CPU (mapping the buffer)
and making the content of the buffer valid (coherent) are so tightly coupled in DMA-BUF? 

Alexey

RE: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and dma_buf_vunmap

[Liam Mark]

On Fri, 4 Jan 2019, Skidanov, Alexey wrote:

> 
> 
> > -----Original Message-----
> > From: Liam Mark [mailto:lmark@codeaurora.org]
> > Sent: Friday, January 04, 2019 19:42
> > To: Skidanov, Alexey <alexey.skidanov@intel.com>
> > Cc: Laura Abbott <labbott@redhat.com>; Greg KH <gregkh@linuxfoundation.org>;
> > devel@driverdev.osuosl.org; tkjos@android.com; rve@android.com; linux-
> > kernel@vger.kernel.org; maco@android.com; sumit.semwal@linaro.org
> > Subject: Re: [PATCH v3] staging: android: ion: Add implementation of dma_buf_vmap and
> > dma_buf_vunmap
> > 
> > On Tue, 18 Dec 2018, Alexey Skidanov wrote:
> > 
> > > >>> I was wondering if we could re-open the discussion on adding support to
> > > >>> ION for dma_buf_vmap.
> > > >>> It seems like the patch was not taken as the reviewers wanted more
> > > >>> evidence of an upstream use case.
> > > >>>
> > > >>> Here would be my upstream usage argument for including dma_buf_vmap
> > > >>> support in ION.
> > > >>>
> > > >>> Currently all calls to ion_dma_buf_begin_cpu_access result in the creation
> > > >>> of a kernel mapping for the buffer, unfortunately the resulting call to
> > > >>> alloc_vmap_area can be quite expensive and this has caused a performance
> > > >>> regression for certain clients when they have moved to the new version of
> > > >>> ION.
> > > >>>
> > > >>> The kernel mapping is not actually needed in ion_dma_buf_begin_cpu_access,
> > > >>> and generally isn't needed by clients. So if we remove the creation of the
> > > >>> kernel mapping in ion_dma_buf_begin_cpu_access and only create it when
> > > >>> needed we can speed up the calls to ion_dma_buf_begin_cpu_access.
> > > >>>
> > > >>> An additional benefit of removing the creation of kernel mappings from
> > > >>> ion_dma_buf_begin_cpu_access is that it makes the ION code more secure.
> > > >>> Currently a malicious client could call the DMA_BUF_IOCTL_SYNC IOCTL with
> > > >>> flags DMA_BUF_SYNC_END multiple times to cause the ION buffer kmap_cnt to
> > > >>> go negative which could lead to undesired behavior.
> > > >>>
> > > >>> One disadvantage of the above change is that a kernel mapping is not
> > > >>> already created when a client calls dma_buf_kmap. So the following
> > > >>> dma_buf_kmap contract can't be satisfied.
> > > >>>
> > > >>> /**
> > > >>> * dma_buf_kmap - Map a page of the buffer object into kernel address
> > > >>> space. The
> > > >>> * same restrictions as for kmap and friends apply.
> > > >>> * @dmabuf:	[in]	buffer to map page from.
> > > >>> * @page_num:	[in]	page in PAGE_SIZE units to map.
> > > >>> *
> > > >>> * This call must always succeed, any necessary preparations that might
> > > >>> fail
> > > >>> * need to be done in begin_cpu_access.
> > > >>> */
> > > >>>
> > > >>> But hopefully we can work around this by moving clients to dma_buf_vmap.
> > > >> I think the problem is with the contract. We can't ensure that the call
> > > >> is always succeeds regardless the implementation - any mapping might
> > > >> fail. Probably this is why  *all* clients of dma_buf_kmap() check the
> > > >> return value (so it's safe to return NULL in case of failure).
> > > >>
> > > >
> > > > I think currently the call to dma_buf_kmap will always succeed since the
> > > > DMA-Buf contract requires that the client first successfully call
> > > > dma_buf_begin_cpu_access(), and if dma_buf_begin_cpu_access() succeeds
> > > > then dma_buf_kmap will succeed.
> > > >
> > > >> I would suggest to fix the contract and to keep the dma_buf_kmap()
> > > >> support in ION.
> > > >
> > > > I will leave it to the DMA-Buf maintainers as to whether they want to
> > > > change their contract.
> > > >
> > > > Liam
> > > >
> > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > > > a Linux Foundation Collaborative Project
> > > >
> > >
> > > Ok. We need the list of the clients using the ION in the mainline tree.
> > >
> > 
> > Looks to me like the only functions which might be calling
> > dma_buf_kmap/dma_buf_kunmap on ION buffers are
> > tegra_bo_kmap/tegra_bo_kunmap, I assume Tegra is used in some Android
> > automotive products.
> > 
> > Looks like these functions could be moved over to using
> > dma_buf_vmap/dma_buf_vunmap but it wouldn't be very clean and would add a
> > performance hit.
> > 
> > Liam
> > 
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> 
> I'm a little bit confused. Why making the buffer accessible by CPU (mapping the buffer)
> and making the content of the buffer valid (coherent) are so tightly coupled in DMA-BUF? 
> 

Hi Sumit,

Hope you are feeling better.

I was wondering if you would be open to changes to to the DMA-BUF contract 
so that we can remove the creation of kernel mappings in begin_cpu_access.

This would have the benefit of improving the performance of 
begin_cpu_access and removing the ability for userspace to add and remove 
kernel mappings.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project