Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683185 > unrolled thread

[PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage Vitaly Kuzmichev <vitaly_kuzmichev@mentor.com> - 2017-07-07 15:30 +0200
    Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs         dev->dma_mem breakage Christoph Hellwig <hch@lst.de> - 2017-07-07 16:30 +0200
      Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs  dev->dma_mem breakage Vladimir Murzin <vladimir.murzin@arm.com> - 2017-07-07 17:50 +0200
        Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs  dev->dma_mem breakage Robin Murphy <robin.murphy@arm.com> - 2017-07-07 18:20 +0200
          Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs  dev->dma_mem breakage Vladimir Murzin <vladimir.murzin@arm.com> - 2017-07-07 18:50 +0200
            Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs  dev->dma_mem breakage Robin Murphy <robin.murphy@arm.com> - 2017-07-07 20:00 +0200
              Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs  dev->dma_mem breakage Vladimir Murzin <vladimir.murzin@arm.com> - 2017-07-10 15:50 +0200
          Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs         dev->dma_mem breakage Christoph Hellwig <hch@lst.de> - 2017-07-11 16:20 +0200

#1683185 — [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

From: "George G. Davis" <george_davis@mentor.com>

When a "linux,dma-default" DMA coherent region is defined, the
dma_coherent_default_memory pointer is returned by function
dev_get_coherent_memory() for any struct device *dev which has not
explicitly assigned a dev->dma_mem memory region, i.e. dev->dma_mem is
the NULL pointer. Unfortunately this overlooks the fact that for the
CONFIG_DMA_CMA case, it is also possible that a device may have assigned
a CMA memory region via the dev->cma_area pointer in which case,
the "linux,dma-default" DMA coherent region should not be used.
Since the current code did not consider this case, dev->cma_area regions
are not used when a "linux,dma-default" DMA coherent region is defined.
Instead, memory is allocated from the "linux,dma-default" DMA coherent
region.  This omission could lead to DMA memory allocation failures for
devices such as the "viv,galcore" which require a large contiguous
address space which cannot be supplied by the "linux,dma-default" region
IFF it has been reconfigured to use a CMA memory region. Similar DMA
allocation failures are likely to occur for other devices which require
large memory regions and/or overall allocation requests exceed the size
of the "linux,dma-default" DMA coherent region size.

Fix this by updating the dev_get_coherent_memory() function to return
the NULL pointer if a dev->cma_area region is assigned to a device.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: George G. Davis <george_davis@mentor.com>
Signed-off-by: Vitaly Kuzmichev <vitaly_kuzmichev@mentor.com>
---
 drivers/base/dma-coherent.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index 2ae24c2..acfe140 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -25,6 +25,10 @@ static inline struct dma_coherent_mem *dev_get_coherent_memory(struct device *de
 {
 	if (dev && dev->dma_mem)
 		return dev->dma_mem;
+#ifdef CONFIG_DMA_CMA
+	if (dev && dev->cma_area)
+		return NULL;
+#endif
 	return dma_coherent_default_memory;
 }
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1683217 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

Vladimir,

this is why I really didn't like overloading the current
dma coherent infrastructure with the global pool.

And this new patch seems like piling hacks over hacks.  I think we
should go back and make sure allocations from the global coherent
pool are done by the dma ops implementation, and not before calling
into them - preferably still reusing the common code for it.

Vladimir or Vitaly - can you look into that?

[toc] | [prev] | [next] | [standalone]

#1683261 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

Christoph,

On 07/07/17 15:27, Christoph Hellwig wrote:
> Vladimir,
> 
> this is why I really didn't like overloading the current
> dma coherent infrastructure with the global pool.
> 
> And this new patch seems like piling hacks over hacks.  I think we
> should go back and make sure allocations from the global coherent
> pool are done by the dma ops implementation, and not before calling
> into them - preferably still reusing the common code for it.
> 
> Vladimir or Vitaly - can you look into that?
> 

It is really sad that Vitaly and George did not join to discussions earlier,
so we could avoid being in situation like this.

Likely I'm missing something, but what should happen if device relies on
dma_contiguous_default_area?

Originally, intention behind dma-default was to simplify things, so instead of 

       reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                coherent_dma: linux,dma {
                        compatible = "shared-dma-pool";
                        no-map;
                        reg = <0x78000000 0x800000>;
                };
        };

  
        dev0: dev@12300000 {
                memory-region = <&coherent_dma>;
                /* ... */
        };

        dev1: dev@12500000 {
                memory-region = <&coherent_dma>;
                /* ... */
        };

        dev2: dev@12600000 {
                memory-region = <&coherent_dma>;
                /* ... */
        };

in device tree we could simply have

       reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                coherent_dma: linux,dma {
                        compatible = "shared-dma-pool";
                        no-map;
                        reg = <0x78000000 0x800000>;
                        linux,dma-default;
                };
        };

and that just work in my (NOMMU) case because there is no CMA there...

However, given that dma-default is being overloaded and there are no device
tree users merged yet, I would not object stepping back, reverting "drivers:
dma-coherent: Introduce default DMA pool" and cooperatively rethinking
design/implementation, so every party gets happy. 

The rest of my original patch set should be enough to keep NOMMU working.

Cheers
Vladimir

[toc] | [prev] | [next] | [standalone]

#1683280 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

On 07/07/17 16:40, Vladimir Murzin wrote:
> Christoph,
> 
> On 07/07/17 15:27, Christoph Hellwig wrote:
>> Vladimir,
>>
>> this is why I really didn't like overloading the current
>> dma coherent infrastructure with the global pool.
>>
>> And this new patch seems like piling hacks over hacks.  I think we
>> should go back and make sure allocations from the global coherent
>> pool are done by the dma ops implementation, and not before calling
>> into them - preferably still reusing the common code for it.
>>
>> Vladimir or Vitaly - can you look into that?
>>
> 
> It is really sad that Vitaly and George did not join to discussions earlier,
> so we could avoid being in situation like this.
> 
> Likely I'm missing something, but what should happen if device relies on
> dma_contiguous_default_area?
> 
> Originally, intention behind dma-default was to simplify things, so instead of 
> 
>        reserved-memory {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
> 
>                 coherent_dma: linux,dma {
>                         compatible = "shared-dma-pool";
>                         no-map;
>                         reg = <0x78000000 0x800000>;
>                 };
>         };
> 
>   
>         dev0: dev@12300000 {
>                 memory-region = <&coherent_dma>;
>                 /* ... */
>         };
> 
>         dev1: dev@12500000 {
>                 memory-region = <&coherent_dma>;
>                 /* ... */
>         };
> 
>         dev2: dev@12600000 {
>                 memory-region = <&coherent_dma>;
>                 /* ... */
>         };
> 
> in device tree we could simply have
> 
>        reserved-memory {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
> 
>                 coherent_dma: linux,dma {
>                         compatible = "shared-dma-pool";
>                         no-map;
>                         reg = <0x78000000 0x800000>;
>                         linux,dma-default;
>                 };
>         };
> 
> and that just work in my (NOMMU) case because there is no CMA there...
> 
> However, given that dma-default is being overloaded and there are no device
> tree users merged yet, I would not object stepping back, reverting "drivers:
> dma-coherent: Introduce default DMA pool" and cooperatively rethinking
> design/implementation, so every party gets happy.

I don't think we need to go that far, I reckon it would be clear enough
to just split the per-device vs. global pool interfaces, something like
I've sketched out below (such that the ops->alloc implementation calls
dma_alloc_from_global_coherent() if dma_alloc_from_contiguous() fails).

If anyone wants to take that and run with it, feel free.

Robin.

----->8-----
diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index 640a7e63c453..e6393c6d8359 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -143,6 +143,44 @@ void *dma_mark_declared_memory_occupied(struct
device *dev,
 }
 EXPORT_SYMBOL(dma_mark_declared_memory_occupied);

+static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem,
ssize_t size,
+					dma_addr_t *dma_handle)
+{
+	int order = get_order(size);
+	unsigned long flags;
+	int pageno;
+	int dma_memory_map;
+	void *ret;
+
+	spin_lock_irqsave(&mem->spinlock, flags);
+
+	if (unlikely(size > (mem->size << PAGE_SHIFT)))
+		goto err;
+
+	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
+	if (unlikely(pageno < 0))
+		goto err;
+
+	/*
+	 * Memory was found in the coherent area.
+	 */
+	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
+	ret = mem->virt_base + (pageno << PAGE_SHIFT);
+	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
+	spin_unlock_irqrestore(&mem->spinlock, flags);
+	if (dma_memory_map)
+		memset(ret, 0, size);
+	else
+		memset_io(ret, 0, size);
+
+	return ret;
+
+err:
+	spin_unlock_irqrestore(&mem->spinlock, flags);
+	return NULL;
+}
+EXPORT_SYMBOL(dma_alloc_from_coherent);
+
 /**
  * dma_alloc_from_coherent() - try to allocate memory from the
per-device coherent area
  *
@@ -162,10 +200,6 @@ int dma_alloc_from_coherent(struct device *dev,
ssize_t size,
 				       dma_addr_t *dma_handle, void **ret)
 {
 	struct dma_coherent_mem *mem;
-	int order = get_order(size);
-	unsigned long flags;
-	int pageno;
-	int dma_memory_map;

 	if (!dev)
 		return 0;
@@ -173,32 +207,10 @@ int dma_alloc_from_coherent(struct device *dev,
ssize_t size,
 	if (!mem)
 		return 0;

-	*ret = NULL;
-	spin_lock_irqsave(&mem->spinlock, flags);
+	*ret = __dma_alloc_from_coherent(mem, size, dma_handle);
+	if (*ret)
+		return 1;

-	if (unlikely(size > (mem->size << PAGE_SHIFT)))
-		goto err;
-
-	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
-	if (unlikely(pageno < 0))
-		goto err;
-
-	/*
-	 * Memory was found in the per-device area.
-	 */
-	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
-	*ret = mem->virt_base + (pageno << PAGE_SHIFT);
-	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
-	spin_unlock_irqrestore(&mem->spinlock, flags);
-	if (dma_memory_map)
-		memset(*ret, 0, size);
-	else
-		memset_io(*ret, 0, size);
-
-	return 1;
-
-err:
-	spin_unlock_irqrestore(&mem->spinlock, flags);
 	/*
 	 * In the case where the allocation can not be satisfied from the
 	 * per-device area, try to fall back to generic memory if the
@@ -208,6 +220,15 @@ int dma_alloc_from_coherent(struct device *dev,
ssize_t size,
 }
 EXPORT_SYMBOL(dma_alloc_from_coherent);

+void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle)
+{
+	if (!dma_coherent_default_memory)
+		return NULL;
+
+	return __dma_alloc_from_coherent(dma_coherent_default_memory, size,
+					 handle);
+}
+
 /**
  * dma_release_from_coherent() - try to free the memory allocated from
per-device coherent memory pool
  * @dev:	device from which the memory was allocated

[toc] | [prev] | [next] | [standalone]

#1683312 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

On 07/07/17 17:06, Robin Murphy wrote:
> On 07/07/17 16:40, Vladimir Murzin wrote:
>> Christoph,
>>
>> On 07/07/17 15:27, Christoph Hellwig wrote:
>>> Vladimir,
>>>
>>> this is why I really didn't like overloading the current
>>> dma coherent infrastructure with the global pool.
>>>
>>> And this new patch seems like piling hacks over hacks.  I think we
>>> should go back and make sure allocations from the global coherent
>>> pool are done by the dma ops implementation, and not before calling
>>> into them - preferably still reusing the common code for it.
>>>
>>> Vladimir or Vitaly - can you look into that?
>>>
>>
>> It is really sad that Vitaly and George did not join to discussions earlier,
>> so we could avoid being in situation like this.
>>
>> Likely I'm missing something, but what should happen if device relies on
>> dma_contiguous_default_area?
>>
>> Originally, intention behind dma-default was to simplify things, so instead of 
>>
>>        reserved-memory {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>                 ranges;
>>
>>                 coherent_dma: linux,dma {
>>                         compatible = "shared-dma-pool";
>>                         no-map;
>>                         reg = <0x78000000 0x800000>;
>>                 };
>>         };
>>
>>   
>>         dev0: dev@12300000 {
>>                 memory-region = <&coherent_dma>;
>>                 /* ... */
>>         };
>>
>>         dev1: dev@12500000 {
>>                 memory-region = <&coherent_dma>;
>>                 /* ... */
>>         };
>>
>>         dev2: dev@12600000 {
>>                 memory-region = <&coherent_dma>;
>>                 /* ... */
>>         };
>>
>> in device tree we could simply have
>>
>>        reserved-memory {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>                 ranges;
>>
>>                 coherent_dma: linux,dma {
>>                         compatible = "shared-dma-pool";
>>                         no-map;
>>                         reg = <0x78000000 0x800000>;
>>                         linux,dma-default;
>>                 };
>>         };
>>
>> and that just work in my (NOMMU) case because there is no CMA there...
>>
>> However, given that dma-default is being overloaded and there are no device
>> tree users merged yet, I would not object stepping back, reverting "drivers:
>> dma-coherent: Introduce default DMA pool" and cooperatively rethinking
>> design/implementation, so every party gets happy.
> 
> I don't think we need to go that far, I reckon it would be clear enough
> to just split the per-device vs. global pool interfaces, something like
> I've sketched out below (such that the ops->alloc implementation calls
> dma_alloc_from_global_coherent() if dma_alloc_from_contiguous() fails).

Would not we need also release and mmap variants?

> 
> If anyone wants to take that and run with it, feel free.
> 
> Robin.
> 
> ----->8-----
> diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
> index 640a7e63c453..e6393c6d8359 100644
> --- a/drivers/base/dma-coherent.c
> +++ b/drivers/base/dma-coherent.c
> @@ -143,6 +143,44 @@ void *dma_mark_declared_memory_occupied(struct
> device *dev,
>  }
>  EXPORT_SYMBOL(dma_mark_declared_memory_occupied);
> 
> +static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem,
> ssize_t size,
> +					dma_addr_t *dma_handle)
> +{
> +	int order = get_order(size);
> +	unsigned long flags;
> +	int pageno;
> +	int dma_memory_map;
> +	void *ret;
> +
> +	spin_lock_irqsave(&mem->spinlock, flags);
> +
> +	if (unlikely(size > (mem->size << PAGE_SHIFT)))
> +		goto err;
> +
> +	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
> +	if (unlikely(pageno < 0))
> +		goto err;
> +
> +	/*
> +	 * Memory was found in the coherent area.
> +	 */
> +	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
> +	ret = mem->virt_base + (pageno << PAGE_SHIFT);
> +	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
> +	spin_unlock_irqrestore(&mem->spinlock, flags);
> +	if (dma_memory_map)
> +		memset(ret, 0, size);
> +	else
> +		memset_io(ret, 0, size);
> +
> +	return ret;
> +
> +err:
> +	spin_unlock_irqrestore(&mem->spinlock, flags);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(dma_alloc_from_coherent);

We already export dma_alloc_from_coherent

> +
>  /**
>   * dma_alloc_from_coherent() - try to allocate memory from the
> per-device coherent area
>   *
> @@ -162,10 +200,6 @@ int dma_alloc_from_coherent(struct device *dev,
> ssize_t size,
>  				       dma_addr_t *dma_handle, void **ret)
>  {
>  	struct dma_coherent_mem *mem;
> -	int order = get_order(size);
> -	unsigned long flags;
> -	int pageno;
> -	int dma_memory_map;
> 
>  	if (!dev)
>  		return 0;
> @@ -173,32 +207,10 @@ int dma_alloc_from_coherent(struct device *dev,
> ssize_t size,
>  	if (!mem)
>  		return 0;
> 
> -	*ret = NULL;
> -	spin_lock_irqsave(&mem->spinlock, flags);
> +	*ret = __dma_alloc_from_coherent(mem, size, dma_handle);
> +	if (*ret)
> +		return 1;
> 
> -	if (unlikely(size > (mem->size << PAGE_SHIFT)))
> -		goto err;
> -
> -	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
> -	if (unlikely(pageno < 0))
> -		goto err;
> -
> -	/*
> -	 * Memory was found in the per-device area.
> -	 */
> -	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
> -	*ret = mem->virt_base + (pageno << PAGE_SHIFT);
> -	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
> -	spin_unlock_irqrestore(&mem->spinlock, flags);
> -	if (dma_memory_map)
> -		memset(*ret, 0, size);
> -	else
> -		memset_io(*ret, 0, size);
> -
> -	return 1;
> -
> -err:
> -	spin_unlock_irqrestore(&mem->spinlock, flags);
>  	/*
>  	 * In the case where the allocation can not be satisfied from the
>  	 * per-device area, try to fall back to generic memory if the
> @@ -208,6 +220,15 @@ int dma_alloc_from_coherent(struct device *dev,
> ssize_t size,
>  }
>  EXPORT_SYMBOL(dma_alloc_from_coherent);
> 
> +void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle)
> +{
> +	if (!dma_coherent_default_memory)
> +		return NULL;
> +
> +	return __dma_alloc_from_coherent(dma_coherent_default_memory, size,
> +					 handle);
                                         ^^^^^^
                                        dma_handle
> +}
> +

EXPORT_SYMBOL(dma_release_from_coherent); ?


>  /**
>   * dma_release_from_coherent() - try to free the memory allocated from
> per-device coherent memory pool
>   * @dev:	device from which the memory was allocated
> 

Cheers
Vladimir

[toc] | [prev] | [next] | [standalone]

#1683347 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

On 07/07/17 17:44, Vladimir Murzin wrote:
> On 07/07/17 17:06, Robin Murphy wrote:
>> On 07/07/17 16:40, Vladimir Murzin wrote:
>>> Christoph,
>>>
>>> On 07/07/17 15:27, Christoph Hellwig wrote:
>>>> Vladimir,
>>>>
>>>> this is why I really didn't like overloading the current
>>>> dma coherent infrastructure with the global pool.
>>>>
>>>> And this new patch seems like piling hacks over hacks.  I think we
>>>> should go back and make sure allocations from the global coherent
>>>> pool are done by the dma ops implementation, and not before calling
>>>> into them - preferably still reusing the common code for it.
>>>>
>>>> Vladimir or Vitaly - can you look into that?
>>>>
>>>
>>> It is really sad that Vitaly and George did not join to discussions earlier,
>>> so we could avoid being in situation like this.
>>>
>>> Likely I'm missing something, but what should happen if device relies on
>>> dma_contiguous_default_area?
>>>
>>> Originally, intention behind dma-default was to simplify things, so instead of 
>>>
>>>        reserved-memory {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <1>;
>>>                 ranges;
>>>
>>>                 coherent_dma: linux,dma {
>>>                         compatible = "shared-dma-pool";
>>>                         no-map;
>>>                         reg = <0x78000000 0x800000>;
>>>                 };
>>>         };
>>>
>>>   
>>>         dev0: dev@12300000 {
>>>                 memory-region = <&coherent_dma>;
>>>                 /* ... */
>>>         };
>>>
>>>         dev1: dev@12500000 {
>>>                 memory-region = <&coherent_dma>;
>>>                 /* ... */
>>>         };
>>>
>>>         dev2: dev@12600000 {
>>>                 memory-region = <&coherent_dma>;
>>>                 /* ... */
>>>         };
>>>
>>> in device tree we could simply have
>>>
>>>        reserved-memory {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <1>;
>>>                 ranges;
>>>
>>>                 coherent_dma: linux,dma {
>>>                         compatible = "shared-dma-pool";
>>>                         no-map;
>>>                         reg = <0x78000000 0x800000>;
>>>                         linux,dma-default;
>>>                 };
>>>         };
>>>
>>> and that just work in my (NOMMU) case because there is no CMA there...
>>>
>>> However, given that dma-default is being overloaded and there are no device
>>> tree users merged yet, I would not object stepping back, reverting "drivers:
>>> dma-coherent: Introduce default DMA pool" and cooperatively rethinking
>>> design/implementation, so every party gets happy.
>>
>> I don't think we need to go that far, I reckon it would be clear enough
>> to just split the per-device vs. global pool interfaces, something like
>> I've sketched out below (such that the ops->alloc implementation calls
>> dma_alloc_from_global_coherent() if dma_alloc_from_contiguous() fails).
> 
> Would not we need also release and mmap variants?

Sure, that was just bashed out in 2 minutes and diffed into an email on
the assumption that code would help illustrate the general idea I had in
mind more clearly than prose alone. I'm certain it won't even compile
as-is ;)

Robin.

[toc] | [prev] | [next] | [standalone]

#1684245 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

On 07/07/17 18:55, Robin Murphy wrote:
> On 07/07/17 17:44, Vladimir Murzin wrote:
>> On 07/07/17 17:06, Robin Murphy wrote:
>>> On 07/07/17 16:40, Vladimir Murzin wrote:
>>>> Christoph,
>>>>
>>>> On 07/07/17 15:27, Christoph Hellwig wrote:
>>>>> Vladimir,
>>>>>
>>>>> this is why I really didn't like overloading the current
>>>>> dma coherent infrastructure with the global pool.
>>>>>
>>>>> And this new patch seems like piling hacks over hacks.  I think we
>>>>> should go back and make sure allocations from the global coherent
>>>>> pool are done by the dma ops implementation, and not before calling
>>>>> into them - preferably still reusing the common code for it.
>>>>>
>>>>> Vladimir or Vitaly - can you look into that?
>>>>>
>>>>
>>>> It is really sad that Vitaly and George did not join to discussions earlier,
>>>> so we could avoid being in situation like this.
>>>>
>>>> Likely I'm missing something, but what should happen if device relies on
>>>> dma_contiguous_default_area?
>>>>
>>>> Originally, intention behind dma-default was to simplify things, so instead of 
>>>>
>>>>        reserved-memory {
>>>>                 #address-cells = <1>;
>>>>                 #size-cells = <1>;
>>>>                 ranges;
>>>>
>>>>                 coherent_dma: linux,dma {
>>>>                         compatible = "shared-dma-pool";
>>>>                         no-map;
>>>>                         reg = <0x78000000 0x800000>;
>>>>                 };
>>>>         };
>>>>
>>>>   
>>>>         dev0: dev@12300000 {
>>>>                 memory-region = <&coherent_dma>;
>>>>                 /* ... */
>>>>         };
>>>>
>>>>         dev1: dev@12500000 {
>>>>                 memory-region = <&coherent_dma>;
>>>>                 /* ... */
>>>>         };
>>>>
>>>>         dev2: dev@12600000 {
>>>>                 memory-region = <&coherent_dma>;
>>>>                 /* ... */
>>>>         };
>>>>
>>>> in device tree we could simply have
>>>>
>>>>        reserved-memory {
>>>>                 #address-cells = <1>;
>>>>                 #size-cells = <1>;
>>>>                 ranges;
>>>>
>>>>                 coherent_dma: linux,dma {
>>>>                         compatible = "shared-dma-pool";
>>>>                         no-map;
>>>>                         reg = <0x78000000 0x800000>;
>>>>                         linux,dma-default;
>>>>                 };
>>>>         };
>>>>
>>>> and that just work in my (NOMMU) case because there is no CMA there...
>>>>
>>>> However, given that dma-default is being overloaded and there are no device
>>>> tree users merged yet, I would not object stepping back, reverting "drivers:
>>>> dma-coherent: Introduce default DMA pool" and cooperatively rethinking
>>>> design/implementation, so every party gets happy.
>>>
>>> I don't think we need to go that far, I reckon it would be clear enough
>>> to just split the per-device vs. global pool interfaces, something like
>>> I've sketched out below (such that the ops->alloc implementation calls
>>> dma_alloc_from_global_coherent() if dma_alloc_from_contiguous() fails).
>>
>> Would not we need also release and mmap variants?
> 
> Sure, that was just bashed out in 2 minutes and diffed into an email on
> the assumption that code would help illustrate the general idea I had in
> mind more clearly than prose alone. I'm certain it won't even compile
> as-is ;)

Ok. I've added missed pieces and even wire-up that with ARM NOMMU and it works
fine for me, but before I go further it'd be handy to know
 1. what does Christoph think of that idea?
 2. what is Vitaly's use case for dma-default?

Cheers
Vladimir

> 
> Robin.
> 

[toc] | [prev] | [next] | [standalone]

#1685053 — Re: [PATCH v2 1/2] drivers: dma-coherent: Fix dev->cma_area vs dev->dma_mem breakage

On Fri, Jul 07, 2017 at 05:06:52PM +0100, Robin Murphy wrote:
> I don't think we need to go that far, I reckon it would be clear enough
> to just split the per-device vs. global pool interfaces, something like
> I've sketched out below (such that the ops->alloc implementation calls
> dma_alloc_from_global_coherent() if dma_alloc_from_contiguous() fails).
> 
> If anyone wants to take that and run with it, feel free.

I like this basic idea.  It also fits into one of my plans for the
4.14 merge window - I want to enhance the lib/dma-noop.c so that
it can use different allocators and mapping helpers, e.g. for
the allocators what makes sense is:

 (1) simple page allocator (as-is)
 (2) CMA
 (3) swiotlb
 (4) the OF coherent allocator from your draft patch

and then for the mapping into phys space we can use

 (1) virto_to_phys (as-is)
 (2) arch helper (e.g. like done in mips plat support)
 (3) maybe some common form of ioremap / vmap instead of various
     duplicates

With that we should be able to cosolidate most direct mapped
dma_ops for architectures that do not require cache flushing into
common code.  As a next step we could think about useful cache
flushing hooks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web