drm: enable uncached DMA optimization for ARM and arm64

Commit Message

Ard Biesheuvel Jan. 24, 2019, 12:06 p.m. UTC

The DRM driver stack is designed to work with cache coherent devices
only, but permits an optimization to be enabled in some cases, where
for some buffers, both the CPU and the GPU use uncached mappings,
removing the need for DMA snooping and allocation in the CPU caches.

The use of uncached GPU mappings relies on the correct implementation
of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
will use cached mappings nonetheless. On x86 platforms, this does not
seem to matter, as uncached CPU mappings will snoop the caches in any
case. However, on ARM and arm64, enabling this optimization on a
platform where NoSnoop is ignored results in loss of coherency, which
breaks correct operation of the device. Since we have no way of
detecting whether NoSnoop works or not, just disable this
optimization entirely for ARM and arm64.

Cc: Christian Koenig <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: David Zhou <[e-mail]>
Cc: Huang Rui <[e-mail]>
Cc: Junwei Zhang <[e-mail]>
Cc: Michel Daenzer <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: Michael Ellerman <[e-mail]>
Cc: Benjamin Herrenschmidt <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: Robin Murphy <[e-mail]>
Cc: amd-gfx list <[e-mail]>
Cc: dri-devel <[e-mail]>
Reported-by: Carsten Haitzler <[e-mail]>
Signed-off-by: Ard Biesheuvel <[e-mail]>
---
 include/drm/drm_cache.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Christian König Jan. 24, 2019, 12:31 p.m. UTC | #1

Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> The DRM driver stack is designed to work with cache coherent devices
> only, but permits an optimization to be enabled in some cases, where
> for some buffers, both the CPU and the GPU use uncached mappings,
> removing the need for DMA snooping and allocation in the CPU caches.
>
> The use of uncached GPU mappings relies on the correct implementation
> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> will use cached mappings nonetheless. On x86 platforms, this does not
> seem to matter, as uncached CPU mappings will snoop the caches in any
> case. However, on ARM and arm64, enabling this optimization on a
> platform where NoSnoop is ignored results in loss of coherency, which
> breaks correct operation of the device. Since we have no way of
> detecting whether NoSnoop works or not, just disable this
> optimization entirely for ARM and arm64.
>
> Cc: Christian Koenig <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: David Zhou <[e-mail]>
> Cc: Huang Rui <[e-mail]>
> Cc: Junwei Zhang <[e-mail]>
> Cc: Michel Daenzer <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: Michael Ellerman <[e-mail]>
> Cc: Benjamin Herrenschmidt <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Robin Murphy <[e-mail]>
> Cc: amd-gfx list <[e-mail]>
> Cc: dri-devel <[e-mail]>
> Reported-by: Carsten Haitzler <[e-mail]>
> Signed-off-by: Ard Biesheuvel <[e-mail]>

The subject line should probably read "disable uncached...".

With that fixed the patch is Reviewed-by: Christian König 
<[e-mail]>.

Regards,
Christian.

> ---
>   include/drm/drm_cache.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> index bfe1639df02d..97fc498dc767 100644
> --- a/include/drm/drm_cache.h
> +++ b/include/drm/drm_cache.h
> @@ -47,6 +47,24 @@ static inline bool drm_arch_can_wc_memory(void)
>   	return false;
>   #elif defined(CONFIG_MIPS) && defined(CONFIG_CPU_LOONGSON3)
>   	return false;
> +#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +	/*
> +	 * The DRM driver stack is designed to work with cache coherent devices
> +	 * only, but permits an optimization to be enabled in some cases, where
> +	 * for some buffers, both the CPU and the GPU use uncached mappings,
> +	 * removing the need for DMA snooping and allocation in the CPU caches.
> +	 *
> +	 * The use of uncached GPU mappings relies on the correct implementation
> +	 * of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> +	 * will use cached mappings nonetheless. On x86 platforms, this does not
> +	 * seem to matter, as uncached CPU mappings will snoop the caches in any
> +	 * case. However, on ARM and arm64, enabling this optimization on a
> +	 * platform where NoSnoop is ignored results in loss of coherency, which
> +	 * breaks correct operation of the device. Since we have no way of
> +	 * detecting whether NoSnoop works or not, just disable this
> +	 * optimization entirely for ARM and arm64.
> +	 */
> +	return false;
>   #else
>   	return true;
>   #endif

Ard Biesheuvel Jan. 24, 2019, 12:32 p.m. UTC | #2

On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
<[e-mail]> wrote:
>
> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > The DRM driver stack is designed to work with cache coherent devices
> > only, but permits an optimization to be enabled in some cases, where
> > for some buffers, both the CPU and the GPU use uncached mappings,
> > removing the need for DMA snooping and allocation in the CPU caches.
> >
> > The use of uncached GPU mappings relies on the correct implementation
> > of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > will use cached mappings nonetheless. On x86 platforms, this does not
> > seem to matter, as uncached CPU mappings will snoop the caches in any
> > case. However, on ARM and arm64, enabling this optimization on a
> > platform where NoSnoop is ignored results in loss of coherency, which
> > breaks correct operation of the device. Since we have no way of
> > detecting whether NoSnoop works or not, just disable this
> > optimization entirely for ARM and arm64.
> >
> > Cc: Christian Koenig <[e-mail]>
> > Cc: Alex Deucher <[e-mail]>
> > Cc: David Zhou <[e-mail]>
> > Cc: Huang Rui <[e-mail]>
> > Cc: Junwei Zhang <[e-mail]>
> > Cc: Michel Daenzer <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Sean Paul <[e-mail]>
> > Cc: Michael Ellerman <[e-mail]>
> > Cc: Benjamin Herrenschmidt <[e-mail]>
> > Cc: Will Deacon <[e-mail]>
> > Cc: Christoph Hellwig <[e-mail]>
> > Cc: Robin Murphy <[e-mail]>
> > Cc: amd-gfx list <[e-mail]>
> > Cc: dri-devel <[e-mail]>
> > Reported-by: Carsten Haitzler <[e-mail]>
> > Signed-off-by: Ard Biesheuvel <[e-mail]>
>
> The subject line should probably read "disable uncached...".
>

Ugh, of course ...

> With that fixed the patch is Reviewed-by: Christian König
> <[e-mail]>.
>

Thanks.

Alex Deucher Jan. 24, 2019, 2:01 p.m. UTC | #3

On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
<[e-mail]> wrote:
>
> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> <[e-mail]> wrote:
> >
> > Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > > The DRM driver stack is designed to work with cache coherent devices
> > > only, but permits an optimization to be enabled in some cases, where
> > > for some buffers, both the CPU and the GPU use uncached mappings,
> > > removing the need for DMA snooping and allocation in the CPU caches.
> > >
> > > The use of uncached GPU mappings relies on the correct implementation
> > > of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > > will use cached mappings nonetheless. On x86 platforms, this does not
> > > seem to matter, as uncached CPU mappings will snoop the caches in any
> > > case. However, on ARM and arm64, enabling this optimization on a
> > > platform where NoSnoop is ignored results in loss of coherency, which
> > > breaks correct operation of the device. Since we have no way of
> > > detecting whether NoSnoop works or not, just disable this
> > > optimization entirely for ARM and arm64.
> > >
> > > Cc: Christian Koenig <[e-mail]>
> > > Cc: Alex Deucher <[e-mail]>
> > > Cc: David Zhou <[e-mail]>
> > > Cc: Huang Rui <[e-mail]>
> > > Cc: Junwei Zhang <[e-mail]>
> > > Cc: Michel Daenzer <[e-mail]>
> > > Cc: David Airlie <[e-mail]>
> > > Cc: Daniel Vetter <[e-mail]>
> > > Cc: Maarten Lankhorst <[e-mail]>
> > > Cc: Maxime Ripard <[e-mail]>
> > > Cc: Sean Paul <[e-mail]>
> > > Cc: Michael Ellerman <[e-mail]>
> > > Cc: Benjamin Herrenschmidt <[e-mail]>
> > > Cc: Will Deacon <[e-mail]>
> > > Cc: Christoph Hellwig <[e-mail]>
> > > Cc: Robin Murphy <[e-mail]>
> > > Cc: amd-gfx list <[e-mail]>
> > > Cc: dri-devel <[e-mail]>
> > > Reported-by: Carsten Haitzler <[e-mail]>
> > > Signed-off-by: Ard Biesheuvel <[e-mail]>
> >
> > The subject line should probably read "disable uncached...".
> >
>
> Ugh, of course ...
>
> > With that fixed the patch is Reviewed-by: Christian König
> > <[e-mail]>.
> >

Same:
Reviewed-by: Alex Deucher <[e-mail]>

>
> Thanks.
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Ard Biesheuvel Jan. 25, 2019, 8:43 a.m. UTC | #4

On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
>
> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> <[e-mail]> wrote:
> >
> > On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> > <[e-mail]> wrote:
> > >
> > > Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > > > The DRM driver stack is designed to work with cache coherent devices
> > > > only, but permits an optimization to be enabled in some cases, where
> > > > for some buffers, both the CPU and the GPU use uncached mappings,
> > > > removing the need for DMA snooping and allocation in the CPU caches.
> > > >
> > > > The use of uncached GPU mappings relies on the correct implementation
> > > > of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > > > will use cached mappings nonetheless. On x86 platforms, this does not
> > > > seem to matter, as uncached CPU mappings will snoop the caches in any
> > > > case. However, on ARM and arm64, enabling this optimization on a
> > > > platform where NoSnoop is ignored results in loss of coherency, which
> > > > breaks correct operation of the device. Since we have no way of
> > > > detecting whether NoSnoop works or not, just disable this
> > > > optimization entirely for ARM and arm64.
> > > >
> > > > Cc: Christian Koenig <[e-mail]>
> > > > Cc: Alex Deucher <[e-mail]>
> > > > Cc: David Zhou <[e-mail]>
> > > > Cc: Huang Rui <[e-mail]>
> > > > Cc: Junwei Zhang <[e-mail]>
> > > > Cc: Michel Daenzer <[e-mail]>
> > > > Cc: David Airlie <[e-mail]>
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > Cc: Maarten Lankhorst <[e-mail]>
> > > > Cc: Maxime Ripard <[e-mail]>
> > > > Cc: Sean Paul <[e-mail]>
> > > > Cc: Michael Ellerman <[e-mail]>
> > > > Cc: Benjamin Herrenschmidt <[e-mail]>
> > > > Cc: Will Deacon <[e-mail]>
> > > > Cc: Christoph Hellwig <[e-mail]>
> > > > Cc: Robin Murphy <[e-mail]>
> > > > Cc: amd-gfx list <[e-mail]>
> > > > Cc: dri-devel <[e-mail]>
> > > > Reported-by: Carsten Haitzler <[e-mail]>
> > > > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > >
> > > The subject line should probably read "disable uncached...".
> > >
> >
> > Ugh, of course ...
> >
> > > With that fixed the patch is Reviewed-by: Christian König
> > > <[e-mail]>.
> > >
>
> Same:
> Reviewed-by: Alex Deucher <[e-mail]>
>

Thanks all

Should I resend the patch with the subject corrected?

Christian König Jan. 25, 2019, 11:30 a.m. UTC | #5

Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
>> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
>> <[e-mail]> wrote:
>>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
>>> <[e-mail]> wrote:
>>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
>>>>> The DRM driver stack is designed to work with cache coherent devices
>>>>> only, but permits an optimization to be enabled in some cases, where
>>>>> for some buffers, both the CPU and the GPU use uncached mappings,
>>>>> removing the need for DMA snooping and allocation in the CPU caches.
>>>>>
>>>>> The use of uncached GPU mappings relies on the correct implementation
>>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
>>>>> will use cached mappings nonetheless. On x86 platforms, this does not
>>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
>>>>> case. However, on ARM and arm64, enabling this optimization on a
>>>>> platform where NoSnoop is ignored results in loss of coherency, which
>>>>> breaks correct operation of the device. Since we have no way of
>>>>> detecting whether NoSnoop works or not, just disable this
>>>>> optimization entirely for ARM and arm64.
>>>>>
>>>>> Cc: Christian Koenig <[e-mail]>
>>>>> Cc: Alex Deucher <[e-mail]>
>>>>> Cc: David Zhou <[e-mail]>
>>>>> Cc: Huang Rui <[e-mail]>
>>>>> Cc: Junwei Zhang <[e-mail]>
>>>>> Cc: Michel Daenzer <[e-mail]>
>>>>> Cc: David Airlie <[e-mail]>
>>>>> Cc: Daniel Vetter <[e-mail]>
>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>> Cc: Sean Paul <[e-mail]>
>>>>> Cc: Michael Ellerman <[e-mail]>
>>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
>>>>> Cc: Will Deacon <[e-mail]>
>>>>> Cc: Christoph Hellwig <[e-mail]>
>>>>> Cc: Robin Murphy <[e-mail]>
>>>>> Cc: amd-gfx list <[e-mail]>
>>>>> Cc: dri-devel <[e-mail]>
>>>>> Reported-by: Carsten Haitzler <[e-mail]>
>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>>> The subject line should probably read "disable uncached...".
>>>>
>>> Ugh, of course ...
>>>
>>>> With that fixed the patch is Reviewed-by: Christian König
>>>> <[e-mail]>.
>>>>
>> Same:
>> Reviewed-by: Alex Deucher <[e-mail]>
>>
> Thanks all
>
> Should I resend the patch with the subject corrected?

I will update the subject line and push it upstream through 
drm-misc-next if nobody objects.

Christian.

> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ard Biesheuvel Jan. 25, 2019, 11:35 a.m. UTC | #6

On Fri, 25 Jan 2019 at 12:30, Christian König
<[e-mail]> wrote:
>
> Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> > On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
> >> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> >> <[e-mail]> wrote:
> >>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> >>> <[e-mail]> wrote:
> >>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> >>>>> The DRM driver stack is designed to work with cache coherent devices
> >>>>> only, but permits an optimization to be enabled in some cases, where
> >>>>> for some buffers, both the CPU and the GPU use uncached mappings,
> >>>>> removing the need for DMA snooping and allocation in the CPU caches.
> >>>>>
> >>>>> The use of uncached GPU mappings relies on the correct implementation
> >>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> >>>>> will use cached mappings nonetheless. On x86 platforms, this does not
> >>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
> >>>>> case. However, on ARM and arm64, enabling this optimization on a
> >>>>> platform where NoSnoop is ignored results in loss of coherency, which
> >>>>> breaks correct operation of the device. Since we have no way of
> >>>>> detecting whether NoSnoop works or not, just disable this
> >>>>> optimization entirely for ARM and arm64.
> >>>>>
> >>>>> Cc: Christian Koenig <[e-mail]>
> >>>>> Cc: Alex Deucher <[e-mail]>
> >>>>> Cc: David Zhou <[e-mail]>
> >>>>> Cc: Huang Rui <[e-mail]>
> >>>>> Cc: Junwei Zhang <[e-mail]>
> >>>>> Cc: Michel Daenzer <[e-mail]>
> >>>>> Cc: David Airlie <[e-mail]>
> >>>>> Cc: Daniel Vetter <[e-mail]>
> >>>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>>> Cc: Maxime Ripard <[e-mail]>
> >>>>> Cc: Sean Paul <[e-mail]>
> >>>>> Cc: Michael Ellerman <[e-mail]>
> >>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
> >>>>> Cc: Will Deacon <[e-mail]>
> >>>>> Cc: Christoph Hellwig <[e-mail]>
> >>>>> Cc: Robin Murphy <[e-mail]>
> >>>>> Cc: amd-gfx list <[e-mail]>
> >>>>> Cc: dri-devel <[e-mail]>
> >>>>> Reported-by: Carsten Haitzler <[e-mail]>
> >>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> >>>> The subject line should probably read "disable uncached...".
> >>>>
> >>> Ugh, of course ...
> >>>
> >>>> With that fixed the patch is Reviewed-by: Christian König
> >>>> <[e-mail]>.
> >>>>
> >> Same:
> >> Reviewed-by: Alex Deucher <[e-mail]>
> >>
> > Thanks all
> >
> > Should I resend the patch with the subject corrected?
>
> I will update the subject line and push it upstream through
> drm-misc-next if nobody objects.
>

Wonderful, thanks.

Ard Biesheuvel Feb. 6, 2019, 5:23 p.m. UTC | #7

On Fri, 25 Jan 2019 at 11:35, Ard Biesheuvel <[e-mail]> wrote:
>
> On Fri, 25 Jan 2019 at 12:30, Christian König
> <[e-mail]> wrote:
> >
> > Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> > > On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
> > >> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> > >> <[e-mail]> wrote:
> > >>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> > >>> <[e-mail]> wrote:
> > >>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > >>>>> The DRM driver stack is designed to work with cache coherent devices
> > >>>>> only, but permits an optimization to be enabled in some cases, where
> > >>>>> for some buffers, both the CPU and the GPU use uncached mappings,
> > >>>>> removing the need for DMA snooping and allocation in the CPU caches.
> > >>>>>
> > >>>>> The use of uncached GPU mappings relies on the correct implementation
> > >>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > >>>>> will use cached mappings nonetheless. On x86 platforms, this does not
> > >>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
> > >>>>> case. However, on ARM and arm64, enabling this optimization on a
> > >>>>> platform where NoSnoop is ignored results in loss of coherency, which
> > >>>>> breaks correct operation of the device. Since we have no way of
> > >>>>> detecting whether NoSnoop works or not, just disable this
> > >>>>> optimization entirely for ARM and arm64.
> > >>>>>
> > >>>>> Cc: Christian Koenig <[e-mail]>
> > >>>>> Cc: Alex Deucher <[e-mail]>
> > >>>>> Cc: David Zhou <[e-mail]>
> > >>>>> Cc: Huang Rui <[e-mail]>
> > >>>>> Cc: Junwei Zhang <[e-mail]>
> > >>>>> Cc: Michel Daenzer <[e-mail]>
> > >>>>> Cc: David Airlie <[e-mail]>
> > >>>>> Cc: Daniel Vetter <[e-mail]>
> > >>>>> Cc: Maarten Lankhorst <[e-mail]>
> > >>>>> Cc: Maxime Ripard <[e-mail]>
> > >>>>> Cc: Sean Paul <[e-mail]>
> > >>>>> Cc: Michael Ellerman <[e-mail]>
> > >>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
> > >>>>> Cc: Will Deacon <[e-mail]>
> > >>>>> Cc: Christoph Hellwig <[e-mail]>
> > >>>>> Cc: Robin Murphy <[e-mail]>
> > >>>>> Cc: amd-gfx list <[e-mail]>
> > >>>>> Cc: dri-devel <[e-mail]>
> > >>>>> Reported-by: Carsten Haitzler <[e-mail]>
> > >>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> > >>>> The subject line should probably read "disable uncached...".
> > >>>>
> > >>> Ugh, of course ...
> > >>>
> > >>>> With that fixed the patch is Reviewed-by: Christian König
> > >>>> <[e-mail]>.
> > >>>>
> > >> Same:
> > >> Reviewed-by: Alex Deucher <[e-mail]>
> > >>
> > > Thanks all
> > >
> > > Should I resend the patch with the subject corrected?
> >
> > I will update the subject line and push it upstream through
> > drm-misc-next if nobody objects.
> >
>
> Wonderful, thanks.

Hi Christian,

Are you still planning to merge this for v5.1?

Thanks,
Ard.

Christian König Feb. 6, 2019, 6:38 p.m. UTC | #8

Am 06.02.19 um 18:23 schrieb Ard Biesheuvel:
> On Fri, 25 Jan 2019 at 11:35, Ard Biesheuvel <[e-mail]> wrote:
>> On Fri, 25 Jan 2019 at 12:30, Christian König
>> <[e-mail]> wrote:
>>> Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
>>>> On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
>>>>> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
>>>>> <[e-mail]> wrote:
>>>>>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
>>>>>> <[e-mail]> wrote:
>>>>>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
>>>>>>>> The DRM driver stack is designed to work with cache coherent devices
>>>>>>>> only, but permits an optimization to be enabled in some cases, where
>>>>>>>> for some buffers, both the CPU and the GPU use uncached mappings,
>>>>>>>> removing the need for DMA snooping and allocation in the CPU caches.
>>>>>>>>
>>>>>>>> The use of uncached GPU mappings relies on the correct implementation
>>>>>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
>>>>>>>> will use cached mappings nonetheless. On x86 platforms, this does not
>>>>>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
>>>>>>>> case. However, on ARM and arm64, enabling this optimization on a
>>>>>>>> platform where NoSnoop is ignored results in loss of coherency, which
>>>>>>>> breaks correct operation of the device. Since we have no way of
>>>>>>>> detecting whether NoSnoop works or not, just disable this
>>>>>>>> optimization entirely for ARM and arm64.
>>>>>>>>
>>>>>>>> Cc: Christian Koenig <[e-mail]>
>>>>>>>> Cc: Alex Deucher <[e-mail]>
>>>>>>>> Cc: David Zhou <[e-mail]>
>>>>>>>> Cc: Huang Rui <[e-mail]>
>>>>>>>> Cc: Junwei Zhang <[e-mail]>
>>>>>>>> Cc: Michel Daenzer <[e-mail]>
>>>>>>>> Cc: David Airlie <[e-mail]>
>>>>>>>> Cc: Daniel Vetter <[e-mail]>
>>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>>>>> Cc: Sean Paul <[e-mail]>
>>>>>>>> Cc: Michael Ellerman <[e-mail]>
>>>>>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
>>>>>>>> Cc: Will Deacon <[e-mail]>
>>>>>>>> Cc: Christoph Hellwig <[e-mail]>
>>>>>>>> Cc: Robin Murphy <[e-mail]>
>>>>>>>> Cc: amd-gfx list <[e-mail]>
>>>>>>>> Cc: dri-devel <[e-mail]>
>>>>>>>> Reported-by: Carsten Haitzler <[e-mail]>
>>>>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>>>>>> The subject line should probably read "disable uncached...".
>>>>>>>
>>>>>> Ugh, of course ...
>>>>>>
>>>>>>> With that fixed the patch is Reviewed-by: Christian König
>>>>>>> <[e-mail]>.
>>>>>>>
>>>>> Same:
>>>>> Reviewed-by: Alex Deucher <[e-mail]>
>>>>>
>>>> Thanks all
>>>>
>>>> Should I resend the patch with the subject corrected?
>>> I will update the subject line and push it upstream through
>>> drm-misc-next if nobody objects.
>>>
>> Wonderful, thanks.
> Hi Christian,
>
> Are you still planning to merge this for v5.1?

My bad, only pushed this to our internal branch, but forgot out 
drm-misc-next.

Fixed now, thanks for the reminder.

Christian.

>
> Thanks,
> Ard.
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Ard Biesheuvel Feb. 7, 2019, 3:20 p.m. UTC | #9

On Wed, 6 Feb 2019 at 19:38, Christian König
<[e-mail]> wrote:
>
> Am 06.02.19 um 18:23 schrieb Ard Biesheuvel:
> > On Fri, 25 Jan 2019 at 11:35, Ard Biesheuvel <[e-mail]> wrote:
> >> On Fri, 25 Jan 2019 at 12:30, Christian König
> >> <[e-mail]> wrote:
> >>> Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> >>>> On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
> >>>>> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> >>>>> <[e-mail]> wrote:
> >>>>>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> >>>>>> <[e-mail]> wrote:
> >>>>>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> >>>>>>>> The DRM driver stack is designed to work with cache coherent devices
> >>>>>>>> only, but permits an optimization to be enabled in some cases, where
> >>>>>>>> for some buffers, both the CPU and the GPU use uncached mappings,
> >>>>>>>> removing the need for DMA snooping and allocation in the CPU caches.
> >>>>>>>>
> >>>>>>>> The use of uncached GPU mappings relies on the correct implementation
> >>>>>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> >>>>>>>> will use cached mappings nonetheless. On x86 platforms, this does not
> >>>>>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
> >>>>>>>> case. However, on ARM and arm64, enabling this optimization on a
> >>>>>>>> platform where NoSnoop is ignored results in loss of coherency, which
> >>>>>>>> breaks correct operation of the device. Since we have no way of
> >>>>>>>> detecting whether NoSnoop works or not, just disable this
> >>>>>>>> optimization entirely for ARM and arm64.
> >>>>>>>>
> >>>>>>>> Cc: Christian Koenig <[e-mail]>
> >>>>>>>> Cc: Alex Deucher <[e-mail]>
> >>>>>>>> Cc: David Zhou <[e-mail]>
> >>>>>>>> Cc: Huang Rui <[e-mail]>
> >>>>>>>> Cc: Junwei Zhang <[e-mail]>
> >>>>>>>> Cc: Michel Daenzer <[e-mail]>
> >>>>>>>> Cc: David Airlie <[e-mail]>
> >>>>>>>> Cc: Daniel Vetter <[e-mail]>
> >>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>>>>>> Cc: Maxime Ripard <[e-mail]>
> >>>>>>>> Cc: Sean Paul <[e-mail]>
> >>>>>>>> Cc: Michael Ellerman <[e-mail]>
> >>>>>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
> >>>>>>>> Cc: Will Deacon <[e-mail]>
> >>>>>>>> Cc: Christoph Hellwig <[e-mail]>
> >>>>>>>> Cc: Robin Murphy <[e-mail]>
> >>>>>>>> Cc: amd-gfx list <[e-mail]>
> >>>>>>>> Cc: dri-devel <[e-mail]>
> >>>>>>>> Reported-by: Carsten Haitzler <[e-mail]>
> >>>>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> >>>>>>> The subject line should probably read "disable uncached...".
> >>>>>>>
> >>>>>> Ugh, of course ...
> >>>>>>
> >>>>>>> With that fixed the patch is Reviewed-by: Christian König
> >>>>>>> <[e-mail]>.
> >>>>>>>
> >>>>> Same:
> >>>>> Reviewed-by: Alex Deucher <[e-mail]>
> >>>>>
> >>>> Thanks all
> >>>>
> >>>> Should I resend the patch with the subject corrected?
> >>> I will update the subject line and push it upstream through
> >>> drm-misc-next if nobody objects.
> >>>
> >> Wonderful, thanks.
> > Hi Christian,
> >
> > Are you still planning to merge this for v5.1?
>
> My bad, only pushed this to our internal branch, but forgot out
> drm-misc-next.
>
> Fixed now, thanks for the reminder.
>

Thanks,

Does anyone mind if I propose this patch for backporting to v4.19 or
earlier once it gets merged for v5.1?

Alex Deucher Feb. 7, 2019, 3:33 p.m. UTC | #10

On Thu, Feb 7, 2019 at 10:20 AM Ard Biesheuvel
<[e-mail]> wrote:
>
> On Wed, 6 Feb 2019 at 19:38, Christian König
> <[e-mail]> wrote:
> >
> > Am 06.02.19 um 18:23 schrieb Ard Biesheuvel:
> > > On Fri, 25 Jan 2019 at 11:35, Ard Biesheuvel <[e-mail]> wrote:
> > >> On Fri, 25 Jan 2019 at 12:30, Christian König
> > >> <[e-mail]> wrote:
> > >>> Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> > >>>> On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
> > >>>>> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> > >>>>> <[e-mail]> wrote:
> > >>>>>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> > >>>>>> <[e-mail]> wrote:
> > >>>>>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > >>>>>>>> The DRM driver stack is designed to work with cache coherent devices
> > >>>>>>>> only, but permits an optimization to be enabled in some cases, where
> > >>>>>>>> for some buffers, both the CPU and the GPU use uncached mappings,
> > >>>>>>>> removing the need for DMA snooping and allocation in the CPU caches.
> > >>>>>>>>
> > >>>>>>>> The use of uncached GPU mappings relies on the correct implementation
> > >>>>>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > >>>>>>>> will use cached mappings nonetheless. On x86 platforms, this does not
> > >>>>>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
> > >>>>>>>> case. However, on ARM and arm64, enabling this optimization on a
> > >>>>>>>> platform where NoSnoop is ignored results in loss of coherency, which
> > >>>>>>>> breaks correct operation of the device. Since we have no way of
> > >>>>>>>> detecting whether NoSnoop works or not, just disable this
> > >>>>>>>> optimization entirely for ARM and arm64.
> > >>>>>>>>
> > >>>>>>>> Cc: Christian Koenig <[e-mail]>
> > >>>>>>>> Cc: Alex Deucher <[e-mail]>
> > >>>>>>>> Cc: David Zhou <[e-mail]>
> > >>>>>>>> Cc: Huang Rui <[e-mail]>
> > >>>>>>>> Cc: Junwei Zhang <[e-mail]>
> > >>>>>>>> Cc: Michel Daenzer <[e-mail]>
> > >>>>>>>> Cc: David Airlie <[e-mail]>
> > >>>>>>>> Cc: Daniel Vetter <[e-mail]>
> > >>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
> > >>>>>>>> Cc: Maxime Ripard <[e-mail]>
> > >>>>>>>> Cc: Sean Paul <[e-mail]>
> > >>>>>>>> Cc: Michael Ellerman <[e-mail]>
> > >>>>>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
> > >>>>>>>> Cc: Will Deacon <[e-mail]>
> > >>>>>>>> Cc: Christoph Hellwig <[e-mail]>
> > >>>>>>>> Cc: Robin Murphy <[e-mail]>
> > >>>>>>>> Cc: amd-gfx list <[e-mail]>
> > >>>>>>>> Cc: dri-devel <[e-mail]>
> > >>>>>>>> Reported-by: Carsten Haitzler <[e-mail]>
> > >>>>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> > >>>>>>> The subject line should probably read "disable uncached...".
> > >>>>>>>
> > >>>>>> Ugh, of course ...
> > >>>>>>
> > >>>>>>> With that fixed the patch is Reviewed-by: Christian König
> > >>>>>>> <[e-mail]>.
> > >>>>>>>
> > >>>>> Same:
> > >>>>> Reviewed-by: Alex Deucher <[e-mail]>
> > >>>>>
> > >>>> Thanks all
> > >>>>
> > >>>> Should I resend the patch with the subject corrected?
> > >>> I will update the subject line and push it upstream through
> > >>> drm-misc-next if nobody objects.
> > >>>
> > >> Wonderful, thanks.
> > > Hi Christian,
> > >
> > > Are you still planning to merge this for v5.1?
> >
> > My bad, only pushed this to our internal branch, but forgot out
> > drm-misc-next.
> >
> > Fixed now, thanks for the reminder.
> >
>
> Thanks,
>
> Does anyone mind if I propose this patch for backporting to v4.19 or
> earlier once it gets merged for v5.1?

Go for it.  I was going to suggest that this should probably go to stable.

Alex

Ard Biesheuvel Feb. 7, 2019, 4:01 p.m. UTC | #11

On Thu, 7 Feb 2019 at 16:33, Alex Deucher <[e-mail]> wrote:
>
> On Thu, Feb 7, 2019 at 10:20 AM Ard Biesheuvel
> <[e-mail]> wrote:
> >
> > On Wed, 6 Feb 2019 at 19:38, Christian König
> > <[e-mail]> wrote:
> > >
> > > Am 06.02.19 um 18:23 schrieb Ard Biesheuvel:
> > > > On Fri, 25 Jan 2019 at 11:35, Ard Biesheuvel <[e-mail]> wrote:
> > > >> On Fri, 25 Jan 2019 at 12:30, Christian König
> > > >> <[e-mail]> wrote:
> > > >>> Am 25.01.19 um 09:43 schrieb Ard Biesheuvel:
> > > >>>> On Thu, 24 Jan 2019 at 15:01, Alex Deucher <[e-mail]> wrote:
> > > >>>>> On Thu, Jan 24, 2019 at 9:00 AM Ard Biesheuvel
> > > >>>>> <[e-mail]> wrote:
> > > >>>>>> On Thu, 24 Jan 2019 at 13:31, Koenig, Christian
> > > >>>>>> <[e-mail]> wrote:
> > > >>>>>>> Am 24.01.19 um 13:06 schrieb Ard Biesheuvel:
> > > >>>>>>>> The DRM driver stack is designed to work with cache coherent devices
> > > >>>>>>>> only, but permits an optimization to be enabled in some cases, where
> > > >>>>>>>> for some buffers, both the CPU and the GPU use uncached mappings,
> > > >>>>>>>> removing the need for DMA snooping and allocation in the CPU caches.
> > > >>>>>>>>
> > > >>>>>>>> The use of uncached GPU mappings relies on the correct implementation
> > > >>>>>>>> of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
> > > >>>>>>>> will use cached mappings nonetheless. On x86 platforms, this does not
> > > >>>>>>>> seem to matter, as uncached CPU mappings will snoop the caches in any
> > > >>>>>>>> case. However, on ARM and arm64, enabling this optimization on a
> > > >>>>>>>> platform where NoSnoop is ignored results in loss of coherency, which
> > > >>>>>>>> breaks correct operation of the device. Since we have no way of
> > > >>>>>>>> detecting whether NoSnoop works or not, just disable this
> > > >>>>>>>> optimization entirely for ARM and arm64.
> > > >>>>>>>>
> > > >>>>>>>> Cc: Christian Koenig <[e-mail]>
> > > >>>>>>>> Cc: Alex Deucher <[e-mail]>
> > > >>>>>>>> Cc: David Zhou <[e-mail]>
> > > >>>>>>>> Cc: Huang Rui <[e-mail]>
> > > >>>>>>>> Cc: Junwei Zhang <[e-mail]>
> > > >>>>>>>> Cc: Michel Daenzer <[e-mail]>
> > > >>>>>>>> Cc: David Airlie <[e-mail]>
> > > >>>>>>>> Cc: Daniel Vetter <[e-mail]>
> > > >>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
> > > >>>>>>>> Cc: Maxime Ripard <[e-mail]>
> > > >>>>>>>> Cc: Sean Paul <[e-mail]>
> > > >>>>>>>> Cc: Michael Ellerman <[e-mail]>
> > > >>>>>>>> Cc: Benjamin Herrenschmidt <[e-mail]>
> > > >>>>>>>> Cc: Will Deacon <[e-mail]>
> > > >>>>>>>> Cc: Christoph Hellwig <[e-mail]>
> > > >>>>>>>> Cc: Robin Murphy <[e-mail]>
> > > >>>>>>>> Cc: amd-gfx list <[e-mail]>
> > > >>>>>>>> Cc: dri-devel <[e-mail]>
> > > >>>>>>>> Reported-by: Carsten Haitzler <[e-mail]>
> > > >>>>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> > > >>>>>>> The subject line should probably read "disable uncached...".
> > > >>>>>>>
> > > >>>>>> Ugh, of course ...
> > > >>>>>>
> > > >>>>>>> With that fixed the patch is Reviewed-by: Christian König
> > > >>>>>>> <[e-mail]>.
> > > >>>>>>>
> > > >>>>> Same:
> > > >>>>> Reviewed-by: Alex Deucher <[e-mail]>
> > > >>>>>
> > > >>>> Thanks all
> > > >>>>
> > > >>>> Should I resend the patch with the subject corrected?
> > > >>> I will update the subject line and push it upstream through
> > > >>> drm-misc-next if nobody objects.
> > > >>>
> > > >> Wonderful, thanks.
> > > > Hi Christian,
> > > >
> > > > Are you still planning to merge this for v5.1?
> > >
> > > My bad, only pushed this to our internal branch, but forgot out
> > > drm-misc-next.
> > >
> > > Fixed now, thanks for the reminder.
> > >
> >
> > Thanks,
> >
> > Does anyone mind if I propose this patch for backporting to v4.19 or
> > earlier once it gets merged for v5.1?
>
> Go for it.  I was going to suggest that this should probably go to stable.
>

Excellent. Note that I actually prefer sending it manually rather than
let Greg or Sasha pick it up automatically, given that they are
usually a bit trigger happy, i.e., patches tend to get backported
before anyone has had a chance to actually check that it doesn't break
anything in mainline.

In other words, please don't add a cc -stable or fixes tag. I will
track it myself instead.

Thanks,
Ard.

Patch

diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
index bfe1639df02d..97fc498dc767 100644
--- a/include/drm/drm_cache.h
+++ b/include/drm/drm_cache.h
@@ -47,6 +47,24 @@  static inline bool drm_arch_can_wc_memory(void)
 	return false;
 #elif defined(CONFIG_MIPS) && defined(CONFIG_CPU_LOONGSON3)
 	return false;
+#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+	/*
+	 * The DRM driver stack is designed to work with cache coherent devices
+	 * only, but permits an optimization to be enabled in some cases, where
+	 * for some buffers, both the CPU and the GPU use uncached mappings,
+	 * removing the need for DMA snooping and allocation in the CPU caches.
+	 *
+	 * The use of uncached GPU mappings relies on the correct implementation
+	 * of the PCIe NoSnoop TLP attribute by the platform, otherwise the GPU
+	 * will use cached mappings nonetheless. On x86 platforms, this does not
+	 * seem to matter, as uncached CPU mappings will snoop the caches in any
+	 * case. However, on ARM and arm64, enabling this optimization on a
+	 * platform where NoSnoop is ignored results in loss of coherency, which
+	 * breaks correct operation of the device. Since we have no way of
+	 * detecting whether NoSnoop works or not, just disable this
+	 * optimization entirely for ARM and arm64.
+	 */
+	return false;
 #else
 	return true;
 #endif