[PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Tao Zhou]

readq/writeq are not supported on all architectures

Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 558fe6d091ed..7eb9e0b9235a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -272,14 +272,10 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
  */
 uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
 {
-	uint64_t ret;
-
 	if ((reg * 4) < adev->rmmio_size)
-		ret = readq(((void __iomem *)adev->rmmio) + (reg * 4));
+		return atomic64_read((atomic64_t *)(adev->rmmio + (reg * 4)));
 	else
 		BUG();
-
-	return ret;
 }
 
 /**
@@ -294,7 +290,7 @@ uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
 void amdgpu_mm_wreg64(struct amdgpu_device *adev, uint32_t reg, uint64_t v)
 {
 	if ((reg * 4) < adev->rmmio_size)
-		writeq(v, ((void __iomem *)adev->rmmio) + (reg * 4));
+		atomic64_set((atomic64_t *)(adev->rmmio + (reg * 4)), v);
 	else
 		BUG();
 }
-- 
2.17.1

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Jisheng Zhang]

On Wed, 7 Aug 2019 10:56:40 +0800 Tao Zhou wrote:


> 
> 
> readq/writeq are not supported on all architectures
> 
> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 558fe6d091ed..7eb9e0b9235a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -272,14 +272,10 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>   */
>  uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
>  {
> -       uint64_t ret;
> -
>         if ((reg * 4) < adev->rmmio_size)
> -               ret = readq(((void __iomem *)adev->rmmio) + (reg * 4));
> +               return atomic64_read((atomic64_t *)(adev->rmmio + (reg * 4)));

atomic64_read doesn't equal to readq on some architectures..

>         else
>                 BUG();
> -
> -       return ret;
>  }
> 
>  /**
> @@ -294,7 +290,7 @@ uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
>  void amdgpu_mm_wreg64(struct amdgpu_device *adev, uint32_t reg, uint64_t v)
>  {
>         if ((reg * 4) < adev->rmmio_size)
> -               writeq(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> +               atomic64_set((atomic64_t *)(adev->rmmio + (reg * 4)), v);
>         else
>                 BUG();
>  }
> --
> 2.17.1

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Alex Deucher]

On Tue, Aug 6, 2019 at 11:31 PM Jisheng Zhang
<Jisheng.Zhang@synaptics.com> wrote:
>
> On Wed, 7 Aug 2019 10:56:40 +0800 Tao Zhou wrote:
>
>
> >
> >
> > readq/writeq are not supported on all architectures
> >
> > Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 558fe6d091ed..7eb9e0b9235a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -272,14 +272,10 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
> >   */
> >  uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
> >  {
> > -       uint64_t ret;
> > -
> >         if ((reg * 4) < adev->rmmio_size)
> > -               ret = readq(((void __iomem *)adev->rmmio) + (reg * 4));
> > +               return atomic64_read((atomic64_t *)(adev->rmmio + (reg * 4)));
>
> atomic64_read doesn't equal to readq on some architectures..

What we really wanted originally was atomic64.  We basically want a
read or write that is guaranteed to be 64 bits at a time.

Alex

>
> >         else
> >                 BUG();
> > -
> > -       return ret;
> >  }
> >
> >  /**
> > @@ -294,7 +290,7 @@ uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
> >  void amdgpu_mm_wreg64(struct amdgpu_device *adev, uint32_t reg, uint64_t v)
> >  {
> >         if ((reg * 4) < adev->rmmio_size)
> > -               writeq(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> > +               atomic64_set((atomic64_t *)(adev->rmmio + (reg * 4)), v);
> >         else
> >                 BUG();
> >  }
> > --
> > 2.17.1
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Alex Deucher]

On Tue, Aug 6, 2019 at 10:56 PM Tao Zhou <tao.zhou1@amd.com> wrote:
>
> readq/writeq are not supported on all architectures
>
> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 558fe6d091ed..7eb9e0b9235a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -272,14 +272,10 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
>   */
>  uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
>  {
> -       uint64_t ret;
> -
>         if ((reg * 4) < adev->rmmio_size)
> -               ret = readq(((void __iomem *)adev->rmmio) + (reg * 4));
> +               return atomic64_read((atomic64_t *)(adev->rmmio + (reg * 4)));
>         else
>                 BUG();
> -
> -       return ret;
>  }
>
>  /**
> @@ -294,7 +290,7 @@ uint64_t amdgpu_mm_rreg64(struct amdgpu_device *adev, uint32_t reg)
>  void amdgpu_mm_wreg64(struct amdgpu_device *adev, uint32_t reg, uint64_t v)
>  {
>         if ((reg * 4) < adev->rmmio_size)
> -               writeq(v, ((void __iomem *)adev->rmmio) + (reg * 4));
> +               atomic64_set((atomic64_t *)(adev->rmmio + (reg * 4)), v);
>         else
>                 BUG();
>  }
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Christoph Hellwig]

On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
> readq/writeq are not supported on all architectures

NAK.  You must not use atomic_* on __iomem (MMIO) memory.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Koenig, Christian]

Am 07.08.19 um 09:08 schrieb Christoph Hellwig:
> On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
>> readq/writeq are not supported on all architectures
> NAK.  You must not use atomic_* on __iomem (MMIO) memory.

Well then what's the right thing to do here?

Essentially writeq/readq doesn't seems to be available on all 
architectures either.

Regards,
Christian.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Christoph Hellwig]

On Wed, Aug 07, 2019 at 08:53:25AM +0000, Koenig, Christian wrote:
> Am 07.08.19 um 09:08 schrieb Christoph Hellwig:
> > On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
> >> readq/writeq are not supported on all architectures
> > NAK.  You must not use atomic_* on __iomem (MMIO) memory.
> 
> Well then what's the right thing to do here?
> 
> Essentially writeq/readq doesn't seems to be available on all 
> architectures either.

writeq/readq are provided whenever the CPU actually supports 64-bit
atomic loads and stores.  If it doesn't provide them atomic64* is
not going to be atomic vs the I/O device either.  And that is on top
of the fact that for various architectures you can't simply use
plain loads and stores on MMIO memory to start with, which is why
we have the special accessors and the __iomem annotation.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Koenig, Christian]

Am 07.08.19 um 12:41 schrieb Christoph Hellwig:
> On Wed, Aug 07, 2019 at 08:53:25AM +0000, Koenig, Christian wrote:
>> Am 07.08.19 um 09:08 schrieb Christoph Hellwig:
>>> On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
>>>> readq/writeq are not supported on all architectures
>>> NAK.  You must not use atomic_* on __iomem (MMIO) memory.
>> Well then what's the right thing to do here?
>>
>> Essentially writeq/readq doesn't seems to be available on all
>> architectures either.
> writeq/readq are provided whenever the CPU actually supports 64-bit
> atomic loads and stores.

Is there a config option which we can make the driver depend on?

I mean that ARM doesn't support 64bit atomic loads and stores on MMIO is 
quite a boomer for us.

Toa do you of hand know what we actually need the 64bit atomic stores 
for? E.g. what is the hardware background?

Regards,
Christian.

> If it doesn't provide them atomic64* is
> not going to be atomic vs the I/O device either.  And that is on top
> of the fact that for various architectures you can't simply use
> plain loads and stores on MMIO memory to start with, which is why
> we have the special accessors and the __iomem annotation.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
> Am 07.08.19 um 12:41 schrieb Christoph Hellwig:

> > writeq/readq are provided whenever the CPU actually supports 64-bit
> > atomic loads and stores.

> Is there a config option which we can make the driver depend on?

64BIT should do the trick.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Koenig, Christian]

Am 07.08.19 um 14:59 schrieb Mark Brown:
> On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
>> Am 07.08.19 um 12:41 schrieb Christoph Hellwig:
>>> writeq/readq are provided whenever the CPU actually supports 64-bit
>>> atomic loads and stores.
>> Is there a config option which we can make the driver depend on?
> 64BIT should do the trick.

That doesn't work because 32bit x86 does support writeq/readq as far as 
I know.

Christian.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Christoph Hellwig]

On Wed, Aug 07, 2019 at 01:00:48PM +0000, Koenig, Christian wrote:
> Am 07.08.19 um 14:59 schrieb Mark Brown:
> > On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
> >> Am 07.08.19 um 12:41 schrieb Christoph Hellwig:
> >>> writeq/readq are provided whenever the CPU actually supports 64-bit
> >>> atomic loads and stores.
> >> Is there a config option which we can make the driver depend on?
> > 64BIT should do the trick.
> 
> That doesn't work because 32bit x86 does support writeq/readq as far as 
> I know.

I also had a vague memory that x86-32 did support it with SSE, but
I can't actually find any traces of that support.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Christoph Hellwig]

On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
> >> Essentially writeq/readq doesn't seems to be available on all
> >> architectures either.
> > writeq/readq are provided whenever the CPU actually supports 64-bit
> > atomic loads and stores.
> 
> Is there a config option which we can make the driver depend on?
> 
> I mean that ARM doesn't support 64bit atomic loads and stores on MMIO is 
> quite a boomer for us.

The model is to cheack if readq/writeq are defined, and if not to
include the one of io-64-nonatomic-hi-lo.h or io-64-nonatomic-lo-hi.h.
The reason for that is that hardware is supposed to be able to deal with
two 32-bit writes, but it depends on the hardware if the lower or upper
half is what commits the write.

The only 32-bit platform that claims support for readq/writeq is sh,
and I have doubts if that actually works as expected.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Koenig, Christian]

Am 07.08.19 um 15:00 schrieb Christoph Hellwig:
> On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
>>>> Essentially writeq/readq doesn't seems to be available on all
>>>> architectures either.
>>> writeq/readq are provided whenever the CPU actually supports 64-bit
>>> atomic loads and stores.
>> Is there a config option which we can make the driver depend on?
>>
>> I mean that ARM doesn't support 64bit atomic loads and stores on MMIO is
>> quite a boomer for us.
> The model is to cheack if readq/writeq are defined, and if not to
> include the one of io-64-nonatomic-hi-lo.h or io-64-nonatomic-lo-hi.h.
> The reason for that is that hardware is supposed to be able to deal with
> two 32-bit writes, but it depends on the hardware if the lower or upper
> half is what commits the write.

Read, but as I understood Tao change this is not the case here. 
Otherwise we would just use our WREG32/RREG32 macros in the driver.

Tao, please explain why exactly we need the WREG64/RREG64 change which 
caused this.

Christian.

>
> The only 32-bit platform that claims support for readq/writeq is sh,
> and I have doubts if that actually works as expected.

Re: [PATCH] drm/amdgpu: replace readq/writeq with atomic64 operations

[Alex Deucher]

On Wed, Aug 7, 2019 at 9:03 AM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 07.08.19 um 15:00 schrieb Christoph Hellwig:
> > On Wed, Aug 07, 2019 at 10:55:01AM +0000, Koenig, Christian wrote:
> >>>> Essentially writeq/readq doesn't seems to be available on all
> >>>> architectures either.
> >>> writeq/readq are provided whenever the CPU actually supports 64-bit
> >>> atomic loads and stores.
> >> Is there a config option which we can make the driver depend on?
> >>
> >> I mean that ARM doesn't support 64bit atomic loads and stores on MMIO is
> >> quite a boomer for us.
> > The model is to cheack if readq/writeq are defined, and if not to
> > include the one of io-64-nonatomic-hi-lo.h or io-64-nonatomic-lo-hi.h.
> > The reason for that is that hardware is supposed to be able to deal with
> > two 32-bit writes, but it depends on the hardware if the lower or upper
> > half is what commits the write.
>
> Read, but as I understood Tao change this is not the case here.
> Otherwise we would just use our WREG32/RREG32 macros in the driver.
>
> Tao, please explain why exactly we need the WREG64/RREG64 change which
> caused this.

We use this for doorbells as well which is also MMIO.  Basically we
have the requirement to read or write the full 64 bits in one
operation.  E.g., for 64-bit doorbells, the entire register is the
trigger so if we do it as two writes, we'll miss half the update.  In
the case of some error counter registers, reading the register will
clear the value so we need to read out the full value or we lose the
half the value.  This works properly on x86 and AMD64.

Alex

>
> Christian.
>
> >
> > The only 32-bit platform that claims support for readq/writeq is sh,
> > and I have doubts if that actually works as expected.
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: drm/amdgpu: replace readq/writeq with atomic64 operations

[Guenter Roeck]

On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
> readq/writeq are not supported on all architectures
> 
> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Regarding the claim that this would work for 32-bit x86 builds:

make ARCH=i386 allmodconfig
make ARCH=i386 drivers/gpu/drm/amd/amdgpu/amdgpu_device.o

results in:

  ...
  CC [M]  drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_rreg64’:
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:279:9: error: implicit declaration of function ‘readq’;

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_wreg64’:
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:298:3: error: implicit declaration of function ‘writeq’

This is with next-20190808.

Guenter

Re: drm/amdgpu: replace readq/writeq with atomic64 operations

[Alex Deucher]

On Thu, Aug 8, 2019 at 3:31 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
> > readq/writeq are not supported on all architectures
> >
> > Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Regarding the claim that this would work for 32-bit x86 builds:

I wasn't talking about readq/writeq, I was talking about the atomic64
interfaces.

Alex

>
> make ARCH=i386 allmodconfig
> make ARCH=i386 drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
>
> results in:
>
>   ...
>   CC [M]  drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_rreg64’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:279:9: error: implicit declaration of function ‘readq’;
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_wreg64’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:298:3: error: implicit declaration of function ‘writeq’
>
> This is with next-20190808.
>
> Guenter
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: drm/amdgpu: replace readq/writeq with atomic64 operations

[Koenig, Christian]

Am 08.08.19 um 21:33 schrieb Alex Deucher:
> On Thu, Aug 8, 2019 at 3:31 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> On Wed, Aug 07, 2019 at 10:56:40AM +0800, Tao Zhou wrote:
>>> readq/writeq are not supported on all architectures
>>>
>>> Signed-off-by: Tao Zhou <tao.zhou1@amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> Regarding the claim that this would work for 32-bit x86 builds:
> I wasn't talking about readq/writeq, I was talking about the atomic64
> interfaces.

On quite a bunch of architectures atomic64 operations are actually 
implemented with spinlocks or other architecture depended special handling.

So the approach of casting an iomem pointer to an atomic64 and then hope 
for the best is actually completely nonsense.

If the hardware really needs a single 64bit write for doorbells or 
registers, then we absolutely need to limit the driver to 64bit 
architectures.

If the hardware doesn't need 64bit writes we should actually always use 
two 32bit writes to not run into random and hard to debug problems 
because of this.

Christian.

>
> Alex
>
>> make ARCH=i386 allmodconfig
>> make ARCH=i386 drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
>>
>> results in:
>>
>>    ...
>>    CC [M]  drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_rreg64’:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:279:9: error: implicit declaration of function ‘readq’;
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_mm_wreg64’:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:298:3: error: implicit declaration of function ‘writeq’
>>
>> This is with next-20190808.
>>
>> Guenter
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx