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

Groups > linux.kernel > #1675876 > unrolled thread

[PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations

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 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann <arnd@arndb.de> - 2017-06-27 17:10 +0200
    Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA  allocations Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2017-06-27 21:50 +0200
      Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations Arnd Bergmann <arnd@arndb.de> - 2017-06-27 22:20 +0200

#1675876 — [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations

In venus_boot(), we pass a pointer to a phys_addr_t
into dmam_alloc_coherent, which the compiler warns about:

platform/qcom/venus/firmware.c: In function 'venus_boot':
platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]

The returned DMA address is later passed on to a function that
takes a phys_addr_t, so it's clearly wrong to use the DMA
mapping interface here: the memory may be uncached, or the
address may be completely wrong if there is an IOMMU connected
to the device.

My interpretation is that using dmam_alloc_coherent() had two
purposes:

 a) get a chunk of consecutive memory that may be larger than
    the limit for kmalloc()

 b) use the devres infrastructure to simplify the unwinding
    in the error case.

I think ideally we'd use a devres-based version of
alloc_pages_exact() here, but since that doesn't exist,
let's use devm_get_free_pages() instead. This wastes a little
memory as the size gets rounded up to a power of two, but
is otherwise harmless. If we want to save memory here, calling
devm_free_pages() to release the memory once it is no longer
needed is probably better anyway.

Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
The same problem exists in the drm driver, as of commit 7c65817e6d38
("drm/msm: gpu: Enable zap shader for A5XX"), and I submitted the
same patch for that already.
---
 drivers/media/platform/qcom/venus/firmware.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 1b1a4f355918..76edb9f60311 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -60,11 +60,13 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname)
 
 	mem_size = VENUS_FW_MEM_SIZE;
 
-	mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL);
+	mem_va = (void *)devm_get_free_pages(parent, GFP_KERNEL,
+					     get_order(mem_size));
 	if (!mem_va) {
 		ret = -ENOMEM;
 		goto err_unreg_device;
 	}
+	mem_phys = virt_to_phys(mem_va);
 
 	ret = request_firmware(&mdt, fwname, fw_dev);
 	if (ret < 0)
-- 
2.9.0

[toc] | [next] | [standalone]

#1676115 — Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations

Hi Arnd,

On 27.06.2017 18:02, Arnd Bergmann wrote:
> In venus_boot(), we pass a pointer to a phys_addr_t
> into dmam_alloc_coherent, which the compiler warns about:
> 
> platform/qcom/venus/firmware.c: In function 'venus_boot':
> platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 
> The returned DMA address is later passed on to a function that
> takes a phys_addr_t, so it's clearly wrong to use the DMA
> mapping interface here: the memory may be uncached, or the
> address may be completely wrong if there is an IOMMU connected
> to the device.
> 
> My interpretation is that using dmam_alloc_coherent() had two
> purposes:
> 
>   a) get a chunk of consecutive memory that may be larger than
>      the limit for kmalloc()
> 
>   b) use the devres infrastructure to simplify the unwinding
>      in the error case.

The intension here is to use per-device memory which is removed from 
kernel allocator, that memory is used by remote processor (Venus) for 
its code section and system memory, the memory must not be mapped to 
kernel to avoid any cache issues.

As the memory in subject is reserved per-device memory the only legal 
way to allocate it is by dmam_alloc_coherent() -> 
dma_alloc_from_coherent().

For me the confusion comes from phys_addr_t which is passed to 
qcom_mdt_load() and then the address passed to qcom_scm_pas_mem_setup() 
which probably protects that physical memory. And the tz really expects 
physical address.

The only solution I see is by casting dma_addr_t to phys_addr_t. Yes it 
is ugly but what is proper solution then?

> 
> I think ideally we'd use a devres-based version of
> alloc_pages_exact() here, but since that doesn't exist,
> let's use devm_get_free_pages() instead. This wastes a little
> memory as the size gets rounded up to a power of two, but
> is otherwise harmless. If we want to save memory here, calling
> devm_free_pages() to release the memory once it is no longer
> needed is probably better anyway.
> 
> Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> The same problem exists in the drm driver, as of commit 7c65817e6d38
> ("drm/msm: gpu: Enable zap shader for A5XX"), and I submitted the
> same patch for that already.
> ---
>   drivers/media/platform/qcom/venus/firmware.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 1b1a4f355918..76edb9f60311 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -60,11 +60,13 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname)
>   
>   	mem_size = VENUS_FW_MEM_SIZE;
>   
> -	mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL);
> +	mem_va = (void *)devm_get_free_pages(parent, GFP_KERNEL,
> +					     get_order(mem_size));
>   	if (!mem_va) {
>   		ret = -ENOMEM;
>   		goto err_unreg_device;
>   	}
> +	mem_phys = virt_to_phys(mem_va);
>   
>   	ret = request_firmware(&mdt, fwname, fw_dev);
>   	if (ret < 0)
> 

regards,
Stan

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

#1676149

On Tue, Jun 27, 2017 at 9:39 PM, Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
> Hi Arnd,
>
> On 27.06.2017 18:02, Arnd Bergmann wrote:
>>
>> In venus_boot(), we pass a pointer to a phys_addr_t
>> into dmam_alloc_coherent, which the compiler warns about:
>>
>> platform/qcom/venus/firmware.c: In function 'venus_boot':
>> platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of
>> 'dmam_alloc_coherent' from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
>>
>> The returned DMA address is later passed on to a function that
>> takes a phys_addr_t, so it's clearly wrong to use the DMA
>> mapping interface here: the memory may be uncached, or the
>> address may be completely wrong if there is an IOMMU connected
>> to the device.
>>
>> My interpretation is that using dmam_alloc_coherent() had two
>> purposes:
>>
>>   a) get a chunk of consecutive memory that may be larger than
>>      the limit for kmalloc()
>>
>>   b) use the devres infrastructure to simplify the unwinding
>>      in the error case.
>
>
> The intension here is to use per-device memory which is removed from kernel
> allocator, that memory is used by remote processor (Venus) for its code
> section and system memory, the memory must not be mapped to kernel to avoid
> any cache issues.
>
> As the memory in subject is reserved per-device memory the only legal way to
> allocate it is by dmam_alloc_coherent() -> dma_alloc_from_coherent().
>
> For me the confusion comes from phys_addr_t which is passed to
> qcom_mdt_load() and then the address passed to qcom_scm_pas_mem_setup()
> which probably protects that physical memory. And the tz really expects
> physical address.
>
> The only solution I see is by casting dma_addr_t to phys_addr_t. Yes it is
> ugly but what is proper solution then?

If you actually have a separate remote processor that accesses this memory,
then qcom_mdt_load() is the wrong interface, as it takes a physical address,
and we need to introduce another interface that can take a DMA address
relative to a particular device.

You cannot cast between the two types because phys_addr_t is an address
as seen from the CPU, and dma_addr_t is seen by a particular device,
and can only be used together with that device pointer.

It looks like the pointer gets passed down to
qcom_scm_call(dev, QCOM_SCM_SVC_PIL,
QCOM_SCM_PAS_MEM_SETUP_CMD, ...), which in turn takes
a 32-bit address, suggesting that this is indeed a dma address for that
device (possibly going through an IOMMU), so maybe it just needs to
all be changed to dma_addr_t.

Is there any official documentation for qcom_scm_call() that clarifies
what address space the arguments are in?

        Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web