Am 04.06.21 um 05:02 schrieb Chen Li:

Also fix some coding issue reported from sparse.

Signed-off-by: Chen Li chenli@uniontech.com

Acked-by: Christian König christian.koenig@amd.com

---

drivers/gpu/drm/radeon/radeon_uvd.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index dfa9fdbe98da..85a1f2c31749 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -152,9 +152,11 @@ int radeon_uvd_init(struct radeon_device *rdev)

	rdev->uvd.fw_header_present = true;

• 	family_id = le32_to_cpu(hdr->ucode_version) & 0xff;
  

• 	version_major = (le32_to_cpu(hdr->ucode_version) >> 24) & 0xff;
  

• 	version_minor = (le32_to_cpu(hdr->ucode_version) >> 8) & 0xff;
  

• 	family_id = (__force u32)(hdr->ucode_version) & 0xff;
  

• 	version_major = (le32_to_cpu((__force __le32)(hdr->ucode_version))
  

• 					 >> 24) & 0xff;
  

• 	version_minor = (le32_to_cpu((__force __le32)(hdr->ucode_version))
  

• 					 >> 8) & 0xff;
  DRM_INFO("Found UVD firmware Version: %u.%u Family ID: %u\n",
  	 version_major, version_minor, family_id);
  
  

@@ -791,17 +793,17 @@ int radeon_uvd_get_create_msg(struct radeon_device *rdev, int ring, return r;

/* stitch together an UVD create msg */

• writel(cpu_to_le32(0x00000de4), &msg[0]);

• writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]); writel(0x0, (void __iomem *)&msg[1]);

• writel(cpu_to_le32(handle), &msg[2]);

• writel((__force u32)cpu_to_le32(handle), &msg[2]); writel(0x0, &msg[3]); writel(0x0, &msg[4]); writel(0x0, &msg[5]); writel(0x0, &msg[6]);

• writel(cpu_to_le32(0x00000780), &msg[7]);
• writel(cpu_to_le32(0x00000440), &msg[8]);

• writel((__force u32)cpu_to_le32(0x00000780), &msg[7]);
• writel((__force u32)cpu_to_le32(0x00000440), &msg[8]); writel(0x0, &msg[9]);

• writel(cpu_to_le32(0x01b37000), &msg[10]);

• writel((__force u32)cpu_to_le32(0x01b37000), &msg[10]); for (i = 11; i < 1024; ++i) writel(0x0, &msg[i]);

@@ -827,9 +829,9 @@ int radeon_uvd_get_destroy_msg(struct radeon_device *rdev, int ring, return r;

/* stitch together an UVD destroy msg */

• writel(cpu_to_le32(0x00000de4), &msg[0]);
• writel(cpu_to_le32(0x00000002), &msg[1]);
• writel(cpu_to_le32(handle), &msg[2]);

• writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]);
• writel((__force u32)cpu_to_le32(0x00000002), &msg[1]);
• writel((__force u32)cpu_to_le32(handle), &msg[2]); writel(0x0, &msg[3]); for (i = 4; i < 1024; ++i) writel(0x0, &msg[i]);

Am 04.06.21 um 05:04 schrieb Chen Li:

I met a gpu addr bug recently and the kernel log tells me the pc is memcpy/memset and link register is radeon_uvd_resume.

As we know, in some architectures, optimized memcpy/memset may not work well on device memory. Trival memcpy_toio/memset_io can fix this problem.

BTW, amdgpu has already done it in: commit ba0b2275a678 ("drm/amdgpu: use memcpy_to/fromio for UVD fw upload"), that's why it has no this issue on the same gpu and platform.

Signed-off-by: Chen Li chenli@uniontech.com

drivers/gpu/drm/radeon/radeon_uvd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 85a1f2c31749..0d6a5cfa2abf 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -288,7 +288,9 @@ int radeon_uvd_resume(struct radeon_device *rdev) if (rdev->uvd.vcpu_bo == NULL) return -EINVAL;

• memcpy(rdev->uvd.cpu_addr, rdev->uvd_fw->data, rdev->uvd_fw->size);

• memcpy_toio((void __iomem *)rdev->uvd.cpu_addr,

• 		rdev->uvd_fw->data,
  

• 		le32_to_cpu((__force __le32)rdev->uvd_fw->size));
  

The le32_to_cpu and coding style looks wrong here. The uvd_fw->size is always in CPU byte order and is used as such.

And please keep in mind that architectures where memcpy() on MMIO doesn't work in the kernel usually doesn't work in userspace as well.

So this is actually now necessary a valid workaround.

Christian.

size = radeon_bo_size(rdev->uvd.vcpu_bo); size -= rdev->uvd_fw->size; @@ -296,7 +298,7 @@ int radeon_uvd_resume(struct radeon_device *rdev) ptr = rdev->uvd.cpu_addr; ptr += rdev->uvd_fw->size;

• memset(ptr, 0, size);

• memset_io((void __iomem *)ptr, 0, size);

  return 0; }

Also fix some coding issues reported from sparse.

Signed-off-by: Chen Li chenli@uniontech.com Acked-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/radeon/radeon_uvd.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index dfa9fdbe98da..85a1f2c31749 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -152,9 +152,11 @@ int radeon_uvd_init(struct radeon_device *rdev)

rdev->uvd.fw_header_present = true;

- family_id = le32_to_cpu(hdr->ucode_version) & 0xff; - version_major = (le32_to_cpu(hdr->ucode_version) >> 24) & 0xff; - version_minor = (le32_to_cpu(hdr->ucode_version) >> 8) & 0xff; + family_id = (__force u32)(hdr->ucode_version) & 0xff; + version_major = (le32_to_cpu((__force __le32)(hdr->ucode_version)) + >> 24) & 0xff; + version_minor = (le32_to_cpu((__force __le32)(hdr->ucode_version)) + >> 8) & 0xff; DRM_INFO("Found UVD firmware Version: %u.%u Family ID: %u\n", version_major, version_minor, family_id);

@@ -791,17 +793,17 @@ int radeon_uvd_get_create_msg(struct radeon_device *rdev, int ring, return r;

/* stitch together an UVD create msg */ - writel(cpu_to_le32(0x00000de4), &msg[0]); + writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]); writel(0x0, (void __iomem *)&msg[1]); - writel(cpu_to_le32(handle), &msg[2]); + writel((__force u32)cpu_to_le32(handle), &msg[2]); writel(0x0, &msg[3]); writel(0x0, &msg[4]); writel(0x0, &msg[5]); writel(0x0, &msg[6]); - writel(cpu_to_le32(0x00000780), &msg[7]); - writel(cpu_to_le32(0x00000440), &msg[8]); + writel((__force u32)cpu_to_le32(0x00000780), &msg[7]); + writel((__force u32)cpu_to_le32(0x00000440), &msg[8]); writel(0x0, &msg[9]); - writel(cpu_to_le32(0x01b37000), &msg[10]); + writel((__force u32)cpu_to_le32(0x01b37000), &msg[10]); for (i = 11; i < 1024; ++i) writel(0x0, &msg[i]);

@@ -827,9 +829,9 @@ int radeon_uvd_get_destroy_msg(struct radeon_device *rdev, int ring, return r;

/* stitch together an UVD destroy msg */ - writel(cpu_to_le32(0x00000de4), &msg[0]); - writel(cpu_to_le32(0x00000002), &msg[1]); - writel(cpu_to_le32(handle), &msg[2]); + writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]); + writel((__force u32)cpu_to_le32(0x00000002), &msg[1]); + writel((__force u32)cpu_to_le32(handle), &msg[2]); writel(0x0, &msg[3]); for (i = 4; i < 1024; ++i) writel(0x0, &msg[i]);

Am 04.06.21 um 09:53 schrieb Chen Li:

I met a gpu addr bug recently and the kernel log tells me the pc is memcpy/memset and link register is radeon_uvd_resume.

As we know, in some architectures, optimized memcpy/memset may not work well on device memory. Trival memcpy_toio/memset_io can fix this problem.

BTW, amdgpu has already done it in: commit ba0b2275a678 ("drm/amdgpu: use memcpy_to/fromio for UVD fw upload"), that's why it has no this issue on the same gpu and platform.

Signed-off-by: Chen Li chenli@uniontech.com

drivers/gpu/drm/radeon/radeon_uvd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 85a1f2c31749..55abf9a9623b 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -288,7 +288,9 @@ int radeon_uvd_resume(struct radeon_device *rdev) if (rdev->uvd.vcpu_bo == NULL) return -EINVAL;

• memcpy(rdev->uvd.cpu_addr, rdev->uvd_fw->data, rdev->uvd_fw->size);

• memcpy_toio((void __iomem *)rdev->uvd.cpu_addr,

• 		rdev->uvd_fw->data,
  

• 		rdev->uvd_fw->size);
  

The coding style still looks wrong here, e.g. it is indented to far to the right and data/size can be on one line.

Apart from that the patch is Reviewed-by: Christian König christian.koenig@amd.com

Regards, Christian.

size = radeon_bo_size(rdev->uvd.vcpu_bo); size -= rdev->uvd_fw->size; @@ -296,7 +298,7 @@ int radeon_uvd_resume(struct radeon_device *rdev) ptr = rdev->uvd.cpu_addr; ptr += rdev->uvd_fw->size;

• memset(ptr, 0, size);

• memset_io((void __iomem *)ptr, 0, size);

  return 0; }

Am 04.06.21 um 10:28 schrieb Chen Li:

On Fri, 04 Jun 2021 16:08:26 +0800, Christian König wrote:

...

Am 04.06.21 um 09:53 schrieb Chen Li:

...

I met a gpu addr bug recently and the kernel log tells me the pc is memcpy/memset and link register is radeon_uvd_resume.

As we know, in some architectures, optimized memcpy/memset may not work well on device memory. Trival memcpy_toio/memset_io can fix this problem.

BTW, amdgpu has already done it in: commit ba0b2275a678 ("drm/amdgpu: use memcpy_to/fromio for UVD fw upload"), that's why it has no this issue on the same gpu and platform.

Signed-off-by: Chen Li chenli@uniontech.com

drivers/gpu/drm/radeon/radeon_uvd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 85a1f2c31749..55abf9a9623b 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -288,7 +288,9 @@ int radeon_uvd_resume(struct radeon_device *rdev) if (rdev->uvd.vcpu_bo == NULL) return -EINVAL;

• memcpy(rdev->uvd.cpu_addr, rdev->uvd_fw->data, rdev->uvd_fw->size);

• memcpy_toio((void __iomem *)rdev->uvd.cpu_addr,

• 		rdev->uvd_fw->data,
  

• 		rdev->uvd_fw->size);
  

The coding style still looks wrong here, e.g. it is indented to far to the right and data/size can be on one line.

It's really werid that the patch before being replyed has not this coding style issue and do always indent the same with previous memcpy(in all of v1, v2 and v3), you can check at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.... Cannot figure out what happened, sorry.

I'll take merge them in single line in the next series, thanks.

It's not much of an issue, just make sure that checkpatch.pl doesn't complain.

Christian.

...

Apart from that the patch is Reviewed-by: Christian König christian.koenig@amd.com

Regards, Christian.

...

 	size = radeon_bo_size(rdev->uvd.vcpu_bo);
size -= rdev->uvd_fw->size;

@@ -296,7 +298,7 @@ int radeon_uvd_resume(struct radeon_device *rdev) ptr = rdev->uvd.cpu_addr; ptr += rdev->uvd_fw->size;

• memset(ptr, 0, size);

• memset_io((void __iomem *)ptr, 0, size); return 0; }

Applied. Thanks!

Alex

On Fri, Jun 4, 2021 at 7:53 AM Chen Li chenli@uniontech.com wrote:

Also fix some coding issues reported from sparse.

Signed-off-by: Chen Li chenli@uniontech.com Acked-by: Christian König christian.koenig@amd.com

---

drivers/gpu/drm/radeon/radeon_uvd.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index dfa9fdbe98da..85a1f2c31749 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -152,9 +152,11 @@ int radeon_uvd_init(struct radeon_device *rdev)

                    rdev->uvd.fw_header_present = true;

•                   family_id = le32_to_cpu(hdr->ucode_version) & 0xff;
  

•                   version_major = (le32_to_cpu(hdr->ucode_version) >> 24) & 0xff;
  

•                   version_minor = (le32_to_cpu(hdr->ucode_version) >> 8) & 0xff;
  

•                   family_id = (__force u32)(hdr->ucode_version) & 0xff;
  

•                   version_major = (le32_to_cpu((__force __le32)(hdr->ucode_version))
  

•                                                    >> 24) & 0xff;
  

•                   version_minor = (le32_to_cpu((__force __le32)(hdr->ucode_version))
  

•                                                    >> 8) & 0xff;
                    DRM_INFO("Found UVD firmware Version: %u.%u Family ID: %u\n",
                             version_major, version_minor, family_id);
  
  

@@ -791,17 +793,17 @@ int radeon_uvd_get_create_msg(struct radeon_device *rdev, int ring, return r;

    /* stitch together an UVD create msg */

•   writel(cpu_to_le32(0x00000de4), &msg[0]);
  

•   writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]);
    writel(0x0, (void __iomem *)&msg[1]);
  

•   writel(cpu_to_le32(handle), &msg[2]);
  

•   writel((__force u32)cpu_to_le32(handle), &msg[2]);
    writel(0x0, &msg[3]);
    writel(0x0, &msg[4]);
    writel(0x0, &msg[5]);
    writel(0x0, &msg[6]);
  

•   writel(cpu_to_le32(0x00000780), &msg[7]);
  

•   writel(cpu_to_le32(0x00000440), &msg[8]);
  

•   writel((__force u32)cpu_to_le32(0x00000780), &msg[7]);
  

•   writel((__force u32)cpu_to_le32(0x00000440), &msg[8]);
    writel(0x0, &msg[9]);
  

•   writel(cpu_to_le32(0x01b37000), &msg[10]);
  

•   writel((__force u32)cpu_to_le32(0x01b37000), &msg[10]);
    for (i = 11; i < 1024; ++i)
            writel(0x0, &msg[i]);
  
  

@@ -827,9 +829,9 @@ int radeon_uvd_get_destroy_msg(struct radeon_device *rdev, int ring, return r;

    /* stitch together an UVD destroy msg */

•   writel(cpu_to_le32(0x00000de4), &msg[0]);
  

•   writel(cpu_to_le32(0x00000002), &msg[1]);
  

•   writel(cpu_to_le32(handle), &msg[2]);
  

•   writel((__force u32)cpu_to_le32(0x00000de4), &msg[0]);
  

•   writel((__force u32)cpu_to_le32(0x00000002), &msg[1]);
  

•   writel((__force u32)cpu_to_le32(handle), &msg[2]);
    writel(0x0, &msg[3]);
    for (i = 4; i < 1024; ++i)
            writel(0x0, &msg[i]);
  

-- 2.31.1

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 04.06.21 um 10:43 schrieb Chen Li:

I met a gpu addr bug recently and the kernel log tells me the pc is memcpy/memset and link register is radeon_uvd_resume.

As we know, in some architectures, optimized memcpy/memset may not work well on device memory. Trival memcpy_toio/memset_io can fix this problem.

BTW, amdgpu has already done it in: commit ba0b2275a678 ("drm/amdgpu: use memcpy_to/fromio for UVD fw upload"), that's why it has no this issue on the same gpu and platform.

Signed-off-by: Chen Li chenli@uniontech.com Reviewed-by: Christian König

Reviewed-by: Christian König christian.koenig@amd.com

Alex will probably now pick them up for upstreaming.

Christian.

---

drivers/gpu/drm/radeon/radeon_uvd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 85a1f2c31749..753da95e6abb 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -288,7 +288,7 @@ int radeon_uvd_resume(struct radeon_device *rdev) if (rdev->uvd.vcpu_bo == NULL) return -EINVAL;

• memcpy(rdev->uvd.cpu_addr, rdev->uvd_fw->data, rdev->uvd_fw->size);

• memcpy_toio((void __iomem *)rdev->uvd.cpu_addr, rdev->uvd_fw->data, rdev->uvd_fw->size);

  size = radeon_bo_size(rdev->uvd.vcpu_bo); size -= rdev->uvd_fw->size;

@@ -296,7 +296,7 @@ int radeon_uvd_resume(struct radeon_device *rdev) ptr = rdev->uvd.cpu_addr; ptr += rdev->uvd_fw->size;

• memset(ptr, 0, size);

• memset_io((void __iomem *)ptr, 0, size);

  return 0; }