On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
> Hello Andy,David,
+ Arnd
I have the same issue on drm-misc-next.
Does Arnaud's fix make sense or should we update/change the way of how
we compile the kernel ?
Regards,
Benjamin
>
>
> I'm facing a compilation error using COMPILE_TEST config,
> could you crosscheck the issue.
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
> commit: b2cd1df66037e7c4697c7e40496bf7e4a5e16a2d :Linux 4.15-rc7
>
> reproduce:
> ---------
> #apt-get install gcc-arm-linux-gnueabi
> #apt-get install gcc-arm-linux-gnueabihf
>
> apply .config (in attachment)
>
> #make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- -j4
>
>
> error:
> -----
> CC drivers/firmware/qcom_scm.o
> drivers/firmware/qcom_scm.c: In function ‘qcom_scm_assign_mem’:
> drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of
> ‘dma_alloc_coherent’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
> ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
> ^
> In file included from drivers/firmware/qcom_scm.c:21:0:
> ./include/linux/dma-mapping.h:549:21: note: expected ‘dma_addr_t * {aka
> long long unsigned int *}’ but argument is of type ‘phys_addr_t * {aka
> unsigned int *}’
> static inline void *dma_alloc_coherent(struct device *dev, size_t size,
> ^
> cc1: some warnings being treated as errors
> scripts/Makefile.build:310: recipe for target
> 'drivers/firmware/qcom_scm.o' failed
> make[2]: *** [drivers/firmware/qcom_scm.o] Error 1
>
> patch to fix compilation issue:
> ------------------------------
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index af4c752..8dfbe61 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -448,7 +448,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t
> mem_sz,
> struct qcom_scm_mem_map_info *mem_to_map;
> phys_addr_t mem_to_map_phys;
> phys_addr_t dest_phys;
> - phys_addr_t ptr_phys;
> + dma_addr_t ptr_phys;
> size_t mem_to_map_sz;
> size_t dest_sz;
> size_t src_sz;
>
> Thanks,
> Arnaud
On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
<[email protected]> wrote:
>
> On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
>> Hello Andy,David,
> + Arnd
>
> I have the same issue on drm-misc-next.
> Does Arnaud's fix make sense or should we update/change the way of how
> we compile the kernel ?
We've hit a couple of bugs with qcom drivers confusing physical addresses
and DMA addresses in the past, usually the drivers were buggy in
some form, and tried to use dma_alloc_coherent() to get a buffer
that gets passed into a firmware interface taking a physical address,
which is of course completely wrong.
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index af4c752..8dfbe61 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -448,7 +448,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t
>> mem_sz,
>> struct qcom_scm_mem_map_info *mem_to_map;
>> phys_addr_t mem_to_map_phys;
>> phys_addr_t dest_phys;
>> - phys_addr_t ptr_phys;
>> + dma_addr_t ptr_phys;
>> size_t mem_to_map_sz;
>> size_t dest_sz;
>> size_t src_sz;
This would be bad: you can basically never have a 'dma_addr_t ptr_phys': it can
be exactly one of 'dma address', 'physical address' or a pointer,
this claims that the
struct member is all three of them.
The proper fix here is to stop using dma_alloc_coherent.
Arnd
2018-01-30 14:25 GMT+01:00 Arnd Bergmann <[email protected]>:
> On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
> <[email protected]> wrote:
>>
>> On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
>>> Hello Andy,David,
>> + Arnd
>>
>> I have the same issue on drm-misc-next.
>> Does Arnaud's fix make sense or should we update/change the way of how
>> we compile the kernel ?
>
> We've hit a couple of bugs with qcom drivers confusing physical addresses
> and DMA addresses in the past, usually the drivers were buggy in
> some form, and tried to use dma_alloc_coherent() to get a buffer
> that gets passed into a firmware interface taking a physical address,
> which is of course completely wrong.
>
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index af4c752..8dfbe61 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -448,7 +448,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t
>>> mem_sz,
>>> struct qcom_scm_mem_map_info *mem_to_map;
>>> phys_addr_t mem_to_map_phys;
>>> phys_addr_t dest_phys;
>>> - phys_addr_t ptr_phys;
>>> + dma_addr_t ptr_phys;
>>> size_t mem_to_map_sz;
>>> size_t dest_sz;
>>> size_t src_sz;
>
> This would be bad: you can basically never have a 'dma_addr_t ptr_phys': it can
> be exactly one of 'dma address', 'physical address' or a pointer,
> this claims that the
> struct member is all three of them.
>
> The proper fix here is to stop using dma_alloc_coherent.
Okay but that doesn't explain why we are the only ones to get an issue
while the parameter
doesn't match function prototype
>
> Arnd
On Tue, Jan 30, 2018 at 3:00 PM, Benjamin Gaignard
<[email protected]> wrote:
> 2018-01-30 14:25 GMT+01:00 Arnd Bergmann <[email protected]>:
>> On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
>> <[email protected]> wrote:
>>>
>>> On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
>>>> Hello Andy,David,
>>> + Arnd
>>>
>>> I have the same issue on drm-misc-next.
>>> Does Arnaud's fix make sense or should we update/change the way of how
>>> we compile the kernel ?
>>
>> We've hit a couple of bugs with qcom drivers confusing physical addresses
>> and DMA addresses in the past, usually the drivers were buggy in
>> some form, and tried to use dma_alloc_coherent() to get a buffer
>> that gets passed into a firmware interface taking a physical address,
>> which is of course completely wrong.
>>
>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>>> index af4c752..8dfbe61 100644
>>>> --- a/drivers/firmware/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom_scm.c
>>>> @@ -448,7 +448,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t
>>>> mem_sz,
>>>> struct qcom_scm_mem_map_info *mem_to_map;
>>>> phys_addr_t mem_to_map_phys;
>>>> phys_addr_t dest_phys;
>>>> - phys_addr_t ptr_phys;
>>>> + dma_addr_t ptr_phys;
>>>> size_t mem_to_map_sz;
>>>> size_t dest_sz;
>>>> size_t src_sz;
>>
>> This would be bad: you can basically never have a 'dma_addr_t ptr_phys': it can
>> be exactly one of 'dma address', 'physical address' or a pointer,
>> this claims that the
>> struct member is all three of them.
>>
>> The proper fix here is to stop using dma_alloc_coherent.
>
> Okay but that doesn't explain why we are the only ones to get an issue
> while the parameter
> doesn't match function prototype
For almost all configurations, dma_addr_t and phys_addr_t are the
same width, and gcc treats them as compatible.
I usually get the warning during randconfig builds, but you seem to
have started with a configuration like this.
Arnd
On Tue 30 Jan 05:25 PST 2018, Arnd Bergmann wrote:
> On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
> <[email protected]> wrote:
> >
> > On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
> >> Hello Andy,David,
> > + Arnd
> >
> > I have the same issue on drm-misc-next.
> > Does Arnaud's fix make sense or should we update/change the way of how
> > we compile the kernel ?
>
> We've hit a couple of bugs with qcom drivers confusing physical addresses
> and DMA addresses in the past, usually the drivers were buggy in
> some form, and tried to use dma_alloc_coherent() to get a buffer
> that gets passed into a firmware interface taking a physical address,
> which is of course completely wrong.
>
Thanks Arnd, for once again using the words "bug" and "completely wrong"
when referring to something that obviously works just fine...
The solution you introduced for venus and adreno relies on static
reservations of system ram, which isn't pretty, but more importantly
isn't viable for the qcom_scm driver.
So, how do I dynamically allocate a chunk of coherent memory?
Preferably with the possibility of unmapping it temporarily from Linux
while passing the buffer into the trusted environment (as any accesses
during the operation might cause access violations).
Regards,
Bjorn
2018-02-03 0:29 GMT+01:00 Bjorn Andersson <[email protected]>:
> On Tue 30 Jan 05:25 PST 2018, Arnd Bergmann wrote:
>
>> On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
>> <[email protected]> wrote:
>> >
>> > On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
>> >> Hello Andy,David,
>> > + Arnd
>> >
>> > I have the same issue on drm-misc-next.
>> > Does Arnaud's fix make sense or should we update/change the way of how
>> > we compile the kernel ?
>>
>> We've hit a couple of bugs with qcom drivers confusing physical addresses
>> and DMA addresses in the past, usually the drivers were buggy in
>> some form, and tried to use dma_alloc_coherent() to get a buffer
>> that gets passed into a firmware interface taking a physical address,
>> which is of course completely wrong.
>>
>
> Thanks Arnd, for once again using the words "bug" and "completely wrong"
> when referring to something that obviously works just fine...
>
I can't judge if using dma_alloc_coherent is correct or not here but, obviously,
the type of the third parameter doesn't match dma_alloc_coherent prototype.
May you consider to fix that ?
>
> The solution you introduced for venus and adreno relies on static
> reservations of system ram, which isn't pretty, but more importantly
> isn't viable for the qcom_scm driver.
>
>
> So, how do I dynamically allocate a chunk of coherent memory?
>
> Preferably with the possibility of unmapping it temporarily from Linux
> while passing the buffer into the trusted environment (as any accesses
> during the operation might cause access violations).
>
> Regards,
> Bjorn
On Sat, Feb 3, 2018 at 12:29 AM, Bjorn Andersson
<[email protected]> wrote:
> On Tue 30 Jan 05:25 PST 2018, Arnd Bergmann wrote:
>
>> On Tue, Jan 30, 2018 at 11:11 AM, Benjamin GAIGNARD
>> <[email protected]> wrote:
>> >
>> > On 01/12/2018 05:11 PM, Arnaud Pouliquen wrote:
>> >> Hello Andy,David,
>> > + Arnd
>> >
>> > I have the same issue on drm-misc-next.
>> > Does Arnaud's fix make sense or should we update/change the way of how
>> > we compile the kernel ?
>>
>> We've hit a couple of bugs with qcom drivers confusing physical addresses
>> and DMA addresses in the past, usually the drivers were buggy in
>> some form, and tried to use dma_alloc_coherent() to get a buffer
>> that gets passed into a firmware interface taking a physical address,
>> which is of course completely wrong.
>>
>
> Thanks Arnd, for once again using the words "bug" and "completely wrong"
> when referring to something that obviously works just fine...
Sorry, you are right my choice of words was uncalled for. I
was getting carried away after seeing Arnaud's suggestion
for changing the variable type but not the name, and got
carried away.
> The solution you introduced for venus and adreno relies on static
> reservations of system ram, which isn't pretty, but more importantly
> isn't viable for the qcom_scm driver.
>
>
> So, how do I dynamically allocate a chunk of coherent memory?
>
> Preferably with the possibility of unmapping it temporarily from Linux
> while passing the buffer into the trusted environment (as any accesses
> during the operation might cause access violations).
Well, there is a fundamental problem here that 'coherent memory'
as such does not exist. The DMA mapping interface is meant to
guarantee coherency between a pair of bus masters, i.e. a CPU
and a device, in a way that is a specific to that pair, without the
driver having to worry about the details.
What the qcom_scm driver does here is to approximate the behavior
it needs by calling the dma mapping interfaces. It happens to work
some of the time, but it's really something else as we are talking
to firmware here and it doesn't have the same semantics that the
dma mapping code provides.
I think what we want here is to come up with a new allocator that
matches the requirements of SCM and that doesn't use dma_addr_t.
For allocating memory that can be unmapped, I think the low-level
CMA allocation can work, which would also make it independent
of the device we are allocating for. Can you say what exactly the
requirements are on the memory buffer? I assume you don't really
want uncached memory, but instead want a buffer that gets flushed
to physical memory before you pass it down so you don't get
a writeback to memory that is inaccessible by the OS, correct?
Arnd