[RFC PATCH 1/3] drm/amdgpu: split amdgpu_device_access_vram() into two small parts

[RFC PATCH 1/3] drm/amdgpu: split amdgpu_device_access_vram() into two small parts

[Kevin Wang]

split amdgpu_device_access_vram()
1. amdgpu_device_mm_access(): using MM_INDEX/MM_DATA to access vram
2. amdgpu_device_aper_access(): using vram aperature to access vram (option)

Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 101 ++++++++++++++++-----
 2 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index d14b4968a026..dd2fc89f5c16 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1102,8 +1102,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 void amdgpu_device_fini(struct amdgpu_device *adev);
 int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
 
+void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
+			     void *buf, size_t size, bool write);
+size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
+				 void *buf, size_t size, bool write);
+
 void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
-			       uint32_t *buf, size_t size, bool write);
+			       void *buf, size_t size, bool write);
 uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
 			    uint32_t reg, uint32_t acc_flags);
 void amdgpu_device_wreg(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index eb1f3f42e00b..4a9a7e4d3908 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -285,7 +285,7 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
  */
 
 /**
- * amdgpu_device_vram_access - read/write a buffer in vram
+ * amdgpu_device_mm_access - access vram by MM_INDEX/MM_DATA
  *
  * @adev: amdgpu_device pointer
  * @pos: offset of the buffer in vram
@@ -293,19 +293,58 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
  * @size: read/write size, sizeof(@buf) must > @size
  * @write: true - write to vram, otherwise - read from vram
  */
-void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
-			       uint32_t *buf, size_t size, bool write)
+void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
+			     void *buf, size_t size, bool write)
 {
 	unsigned long flags;
-	uint32_t hi = ~0;
+	uint32_t hi = ~0, tmp = 0;
+	uint32_t *data = buf;
 	uint64_t last;
 
+	BUG_ON(!IS_ALIGNED(pos, 4) || !IS_ALIGNED(size, 4));
+
+	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+	for (last = pos + size; pos < last; pos += 4) {
+		tmp = pos >> 31;
+
+		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
+		if (tmp != hi) {
+			WREG32_NO_KIQ(mmMM_INDEX_HI, tmp);
+			hi = tmp;
+		}
+		if (write)
+			WREG32_NO_KIQ(mmMM_DATA, *data++);
+		else
+			*data++ = RREG32_NO_KIQ(mmMM_DATA);
+	}
 
+	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+}
+
+/**
+ * amdgpu_device_vram_access - access vram by vram aperature
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ */
+size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
+				 void *buf, size_t size, bool write)
+{
 #ifdef CONFIG_64BIT
+	void __iomem *addr;
+	size_t count = 0;
+	uint64_t last;
+
+	if (!adev->mman.aper_base_kaddr)
+		return 0;
+
 	last = min(pos + size, adev->gmc.visible_vram_size);
 	if (last > pos) {
-		void __iomem *addr = adev->mman.aper_base_kaddr + pos;
-		size_t count = last - pos;
+		addr = adev->mman.aper_base_kaddr + pos;
+		count = last - pos;
 
 		if (write) {
 			memcpy_toio(addr, buf, count);
@@ -317,30 +356,42 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
 			memcpy_fromio(buf, addr, count);
 		}
 
-		if (count == size)
-			return;
-
-		pos += count;
-		buf += count / 4;
-		size -= count;
 	}
+
+	return count;
+#else
+	return 0;
 #endif
+}
 
-	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-	for (last = pos + size; pos < last; pos += 4) {
-		uint32_t tmp = pos >> 31;
+/**
+ * amdgpu_device_vram_access - read/write a buffer in vram
+ *
+ * @adev: amdgpu_device pointer
+ * @pos: offset of the buffer in vram
+ * @buf: virtual address of the buffer in system memory
+ * @size: read/write size, sizeof(@buf) must > @size
+ * @write: true - write to vram, otherwise - read from vram
+ */
+void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
+			       void *buf, size_t size, bool write)
+{
+	size_t count;
 
-		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
-		if (tmp != hi) {
-			WREG32_NO_KIQ(mmMM_INDEX_HI, tmp);
-			hi = tmp;
-		}
-		if (write)
-			WREG32_NO_KIQ(mmMM_DATA, *buf++);
-		else
-			*buf++ = RREG32_NO_KIQ(mmMM_DATA);
+	/* try to using vram apreature to access vram first */
+	count = amdgpu_device_aper_access(adev, pos, buf, size, write);
+	if (count == size)
+		return;
+
+	 if (count && count < size) {
+		pos += count;
+		buf += count;
+		size -= count;
 	}
-	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+
+	/* using MM to access rest vram */
+	if (size)
+		amdgpu_device_mm_access(adev, pos, buf, size, write);
 }
 
 /*
-- 
2.25.1

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

[RFC PATCH 2/3] drm/amdgpu/ttm: replace duplicate code with exiting function

[Kevin Wang]

using exiting function to replace duplicate code blocks in
amdgpu_ttm_vram_write().

Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2aa2eb5de37a..2c98e4345ad3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -2207,7 +2207,6 @@ static ssize_t amdgpu_ttm_vram_write(struct file *f, const char __user *buf,
 		return -ENXIO;
 
 	while (size) {
-		unsigned long flags;
 		uint32_t value;
 
 		if (*pos >= adev->gmc.mc_vram_size)
@@ -2217,11 +2216,7 @@ static ssize_t amdgpu_ttm_vram_write(struct file *f, const char __user *buf,
 		if (r)
 			return r;
 
-		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)*pos) | 0x80000000);
-		WREG32_NO_KIQ(mmMM_INDEX_HI, *pos >> 31);
-		WREG32_NO_KIQ(mmMM_DATA, value);
-		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+		amdgpu_device_vram_access(adev, *pos, &value, 4, true);
 
 		result += 4;
 		buf += 4;
-- 
2.25.1

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

[RFC PATCH 3/3] drm/amdgpu/ttm: optimize vram access in amdgpu_ttm_access_memory()

[Kevin Wang]

1. using vram aper to access vram if possible
2. avoid MM_INDEX/MM_DATA is not working when mmio protect feature is
enabled.

Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 95 +++++++++++++++----------
 1 file changed, 58 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2c98e4345ad3..bf2d50a918e0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1407,6 +1407,60 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 	return ttm_bo_eviction_valuable(bo, place);
 }
 
+static void amdgpu_ttm_vram_mm_access(struct amdgpu_device *adev, loff_t pos,
+				      void *buf, size_t size, bool write)
+{
+	while (size) {
+		uint64_t aligned_pos = ALIGN_DOWN(pos, 4);
+		uint64_t bytes = 4 - (pos & 0x3);
+		uint32_t shift = (pos & 0x3) * 8;
+		uint32_t mask = 0xffffffff << shift;
+		uint32_t value = 0;
+
+		if (size < bytes) {
+			mask &= 0xffffffff >> (bytes - size) * 8;
+			bytes = size;
+		}
+
+		if (mask != 0xffffffff) {
+			amdgpu_device_mm_access(adev, aligned_pos, &value, 4, false);
+			if (write) {
+				value &= ~mask;
+				value |= (*(uint32_t *)buf << shift) & mask;
+				amdgpu_device_mm_access(adev, aligned_pos, &value, 4, true);
+			} else {
+				value = (value & mask) >> shift;
+				memcpy(buf, &value, bytes);
+			}
+		} else {
+			amdgpu_device_mm_access(adev, aligned_pos, buf, 4, write);
+		}
+
+		pos += bytes;
+		buf += bytes;
+		size -= bytes;
+	}
+}
+
+static void amdgpu_ttm_vram_access(struct amdgpu_device *adev, loff_t pos,
+				   void *buf, size_t size, bool write)
+{
+	size_t count;
+
+	count = amdgpu_device_aper_access(adev, pos, buf, size, write);
+	if (count == size)
+		return;
+
+	if (count && count < size) {
+		pos += count;
+		buf += count;
+		size -= count;
+	}
+
+	if (size)
+		amdgpu_ttm_vram_mm_access(adev, pos, buf, size, write);
+}
+
 /**
  * amdgpu_ttm_access_memory - Read or Write memory that backs a buffer object.
  *
@@ -1426,8 +1480,6 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo,
 	struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo);
 	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
 	struct amdgpu_res_cursor cursor;
-	unsigned long flags;
-	uint32_t value = 0;
 	int ret = 0;
 
 	if (bo->mem.mem_type != TTM_PL_VRAM)
@@ -1435,41 +1487,10 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo,
 
 	amdgpu_res_first(&bo->mem, offset, len, &cursor);
 	while (cursor.remaining) {
-		uint64_t aligned_pos = cursor.start & ~(uint64_t)3;
-		uint64_t bytes = 4 - (cursor.start & 3);
-		uint32_t shift = (cursor.start & 3) * 8;
-		uint32_t mask = 0xffffffff << shift;
-
-		if (cursor.size < bytes) {
-			mask &= 0xffffffff >> (bytes - cursor.size) * 8;
-			bytes = cursor.size;
-		}
-
-		if (mask != 0xffffffff) {
-			spin_lock_irqsave(&adev->mmio_idx_lock, flags);
-			WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
-			WREG32_NO_KIQ(mmMM_INDEX_HI, aligned_pos >> 31);
-			value = RREG32_NO_KIQ(mmMM_DATA);
-			if (write) {
-				value &= ~mask;
-				value |= (*(uint32_t *)buf << shift) & mask;
-				WREG32_NO_KIQ(mmMM_DATA, value);
-			}
-			spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
-			if (!write) {
-				value = (value & mask) >> shift;
-				memcpy(buf, &value, bytes);
-			}
-		} else {
-			bytes = cursor.size & ~0x3ULL;
-			amdgpu_device_vram_access(adev, cursor.start,
-						  (uint32_t *)buf, bytes,
-						  write);
-		}
-
-		ret += bytes;
-		buf = (uint8_t *)buf + bytes;
-		amdgpu_res_next(&cursor, bytes);
+		amdgpu_ttm_vram_access(adev, cursor.start, buf, cursor.size, write);
+		ret += cursor.size;
+		buf += cursor.size;
+		amdgpu_res_next(&cursor, cursor.size);
 	}
 
 	return ret;
-- 
2.25.1

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

Re: [RFC PATCH 1/3] drm/amdgpu: split amdgpu_device_access_vram() into two small parts

[Christian König]

Am 15.07.21 um 10:24 schrieb Kevin Wang:
> split amdgpu_device_access_vram()
> 1. amdgpu_device_mm_access(): using MM_INDEX/MM_DATA to access vram
> 2. amdgpu_device_aper_access(): using vram aperature to access vram (option)

Still not the approach I had in mind, but let's move forward we need to 
get this fixed.

>
> Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   7 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 101 ++++++++++++++++-----
>   2 files changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index d14b4968a026..dd2fc89f5c16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1102,8 +1102,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   void amdgpu_device_fini(struct amdgpu_device *adev);
>   int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>   
> +void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> +			     void *buf, size_t size, bool write);
> +size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
> +				 void *buf, size_t size, bool write);
> +
>   void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> -			       uint32_t *buf, size_t size, bool write);
> +			       void *buf, size_t size, bool write);
>   uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
>   			    uint32_t reg, uint32_t acc_flags);
>   void amdgpu_device_wreg(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index eb1f3f42e00b..4a9a7e4d3908 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -285,7 +285,7 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
>    */
>   
>   /**
> - * amdgpu_device_vram_access - read/write a buffer in vram
> + * amdgpu_device_mm_access - access vram by MM_INDEX/MM_DATA
>    *
>    * @adev: amdgpu_device pointer
>    * @pos: offset of the buffer in vram
> @@ -293,19 +293,58 @@ bool amdgpu_device_supports_smart_shift(struct drm_device *dev)
>    * @size: read/write size, sizeof(@buf) must > @size
>    * @write: true - write to vram, otherwise - read from vram
>    */
> -void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> -			       uint32_t *buf, size_t size, bool write)
> +void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> +			     void *buf, size_t size, bool write)
>   {
>   	unsigned long flags;
> -	uint32_t hi = ~0;
> +	uint32_t hi = ~0, tmp = 0;
> +	uint32_t *data = buf;
>   	uint64_t last;
>   
> +	BUG_ON(!IS_ALIGNED(pos, 4) || !IS_ALIGNED(size, 4));
> +
> +	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> +	for (last = pos + size; pos < last; pos += 4) {
> +		tmp = pos >> 31;
> +
> +		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
> +		if (tmp != hi) {
> +			WREG32_NO_KIQ(mmMM_INDEX_HI, tmp);
> +			hi = tmp;
> +		}
> +		if (write)
> +			WREG32_NO_KIQ(mmMM_DATA, *data++);
> +		else
> +			*data++ = RREG32_NO_KIQ(mmMM_DATA);
> +	}
>   
> +	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +}
> +
> +/**
> + * amdgpu_device_vram_access - access vram by vram aperature
> + *
> + * @adev: amdgpu_device pointer
> + * @pos: offset of the buffer in vram
> + * @buf: virtual address of the buffer in system memory
> + * @size: read/write size, sizeof(@buf) must > @size
> + * @write: true - write to vram, otherwise - read from vram

Here we need an one line sentence that the function returns the number 
of bytes transferred.

> + */
> +size_t amdgpu_device_aper_access(struct amdgpu_device *adev, loff_t pos,
> +				 void *buf, size_t size, bool write)
> +{
>   #ifdef CONFIG_64BIT
> +	void __iomem *addr;
> +	size_t count = 0;
> +	uint64_t last;
> +
> +	if (!adev->mman.aper_base_kaddr)
> +		return 0;
> +
>   	last = min(pos + size, adev->gmc.visible_vram_size);
>   	if (last > pos) {
> -		void __iomem *addr = adev->mman.aper_base_kaddr + pos;
> -		size_t count = last - pos;
> +		addr = adev->mman.aper_base_kaddr + pos;
> +		count = last - pos;
>   
>   		if (write) {
>   			memcpy_toio(addr, buf, count);
> @@ -317,30 +356,42 @@ void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
>   			memcpy_fromio(buf, addr, count);
>   		}
>   
> -		if (count == size)
> -			return;
> -
> -		pos += count;
> -		buf += count / 4;
> -		size -= count;
>   	}
> +
> +	return count;
> +#else
> +	return 0;
>   #endif
> +}
>   
> -	spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -	for (last = pos + size; pos < last; pos += 4) {
> -		uint32_t tmp = pos >> 31;
> +/**
> + * amdgpu_device_vram_access - read/write a buffer in vram
> + *
> + * @adev: amdgpu_device pointer
> + * @pos: offset of the buffer in vram
> + * @buf: virtual address of the buffer in system memory
> + * @size: read/write size, sizeof(@buf) must > @size
> + * @write: true - write to vram, otherwise - read from vram
> + */
> +void amdgpu_device_vram_access(struct amdgpu_device *adev, loff_t pos,
> +			       void *buf, size_t size, bool write)
> +{
> +	size_t count;
>   
> -		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)pos) | 0x80000000);
> -		if (tmp != hi) {
> -			WREG32_NO_KIQ(mmMM_INDEX_HI, tmp);
> -			hi = tmp;
> -		}
> -		if (write)
> -			WREG32_NO_KIQ(mmMM_DATA, *buf++);
> -		else
> -			*buf++ = RREG32_NO_KIQ(mmMM_DATA);
> +	/* try to using vram apreature to access vram first */
> +	count = amdgpu_device_aper_access(adev, pos, buf, size, write);
> +	if (count == size)
> +		return;
> +
> +	 if (count && count < size) {
> +		pos += count;
> +		buf += count;
> +		size -= count;
>   	}

Either just do it like this:

size -= amdgpu_device_aper_access()
if (size) ....

Or use ssize_t as return value for amdgpu_device_aper_access() and add 
error handling here, but I don't see the need for this.

Apart from that the patch looks good to me.

Regards,
Christian.

> -	spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +
> +	/* using MM to access rest vram */
> +	if (size)
> +		amdgpu_device_mm_access(adev, pos, buf, size, write);
>   }
>   
>   /*

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

Re: [RFC PATCH 2/3] drm/amdgpu/ttm: replace duplicate code with exiting function

[Christian König]

Am 15.07.21 um 10:24 schrieb Kevin Wang:
> using exiting function to replace duplicate code blocks in
> amdgpu_ttm_vram_write().

NAK, this should intentionally only use the MM path and not the aper path.

But you could use amdgpu_device_mm_access() here now.

Christian.

>
> Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 2aa2eb5de37a..2c98e4345ad3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -2207,7 +2207,6 @@ static ssize_t amdgpu_ttm_vram_write(struct file *f, const char __user *buf,
>   		return -ENXIO;
>   
>   	while (size) {
> -		unsigned long flags;
>   		uint32_t value;
>   
>   		if (*pos >= adev->gmc.mc_vram_size)
> @@ -2217,11 +2216,7 @@ static ssize_t amdgpu_ttm_vram_write(struct file *f, const char __user *buf,
>   		if (r)
>   			return r;
>   
> -		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> -		WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)*pos) | 0x80000000);
> -		WREG32_NO_KIQ(mmMM_INDEX_HI, *pos >> 31);
> -		WREG32_NO_KIQ(mmMM_DATA, value);
> -		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +		amdgpu_device_vram_access(adev, *pos, &value, 4, true);
>   
>   		result += 4;
>   		buf += 4;

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

Re: [RFC PATCH 2/3] drm/amdgpu/ttm: replace duplicate code with exiting function

[Lazar, Lijo]

On 7/15/2021 2:56 PM, Christian König wrote:
> Am 15.07.21 um 10:24 schrieb Kevin Wang:
>> using exiting function to replace duplicate code blocks in
>> amdgpu_ttm_vram_write().
> 
> NAK, this should intentionally only use the MM path and not the aper path.
> 

What about platform configs which don't support HDP?

Thanks,
Lijo

> But you could use amdgpu_device_mm_access() here now.
> 
> Christian.
> 
>>
>> Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 2aa2eb5de37a..2c98e4345ad3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -2207,7 +2207,6 @@ static ssize_t amdgpu_ttm_vram_write(struct file 
>> *f, const char __user *buf,
>>           return -ENXIO;
>>       while (size) {
>> -        unsigned long flags;
>>           uint32_t value;
>>           if (*pos >= adev->gmc.mc_vram_size)
>> @@ -2217,11 +2216,7 @@ static ssize_t amdgpu_ttm_vram_write(struct 
>> file *f, const char __user *buf,
>>           if (r)
>>               return r;
>> -        spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>> -        WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)*pos) | 0x80000000);
>> -        WREG32_NO_KIQ(mmMM_INDEX_HI, *pos >> 31);
>> -        WREG32_NO_KIQ(mmMM_DATA, value);
>> -        spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>> +        amdgpu_device_vram_access(adev, *pos, &value, 4, true);
>>           result += 4;
>>           buf += 4;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu/ttm: replace duplicate code with exiting function

[Christian König]

Am 15.07.21 um 11:29 schrieb Lazar, Lijo:
>
>
> On 7/15/2021 2:56 PM, Christian König wrote:
>> Am 15.07.21 um 10:24 schrieb Kevin Wang:
>>> using exiting function to replace duplicate code blocks in
>>> amdgpu_ttm_vram_write().
>>
>> NAK, this should intentionally only use the MM path and not the aper 
>> path.
>>
>
> What about platform configs which don't support HDP?

Good question, I have no idea.

As far as I know the write interface here intentionally uses the 
MM_INDEX/MM_DATA to bypass the HDP cache.

Regards,
Christian.

>
> Thanks,
> Lijo
>
>> But you could use amdgpu_device_mm_access() here now.
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 +------
>>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 2aa2eb5de37a..2c98e4345ad3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -2207,7 +2207,6 @@ static ssize_t amdgpu_ttm_vram_write(struct 
>>> file *f, const char __user *buf,
>>>           return -ENXIO;
>>>       while (size) {
>>> -        unsigned long flags;
>>>           uint32_t value;
>>>           if (*pos >= adev->gmc.mc_vram_size)
>>> @@ -2217,11 +2216,7 @@ static ssize_t amdgpu_ttm_vram_write(struct 
>>> file *f, const char __user *buf,
>>>           if (r)
>>>               return r;
>>> -        spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>> -        WREG32_NO_KIQ(mmMM_INDEX, ((uint32_t)*pos) | 0x80000000);
>>> -        WREG32_NO_KIQ(mmMM_INDEX_HI, *pos >> 31);
>>> -        WREG32_NO_KIQ(mmMM_DATA, value);
>>> -        spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>> +        amdgpu_device_vram_access(adev, *pos, &value, 4, true);
>>>           result += 4;
>>>           buf += 4;
>>

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