| Message ID | 20190807025640.682-1-tao.zhou1@amd.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c6dddf45402caeadc49dc859fa497cfb98841af4 |
| Headers | show |
| Series | drm/amdgpu: replace readq/writeq with atomic64 operations | expand |
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
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
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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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
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
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
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
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(); }
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(-)