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(); }
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
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.
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.
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 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 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 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
kernel-build-reports@lists.linaro.org
-
Alex Deucher -
Christoph Hellwig -
Guenter Roeck -
Jisheng Zhang -
Koenig, Christian -
Mark Brown -
Tao Zhou