[PATCH 00/12] Patches from amd-kfd-staging

[PATCH 00/12] Patches from amd-kfd-staging

[Felix Kuehling]

Varios cleaned-up and some squashed patches from amd-kfg-staging
that are not necessarily KFD-on-dGPU-specific. The intention is to
minimize differences between amd-kfd-staging and upstream before
reviewing KFD-specific changes for upstreaming.

Patches 7-12 are a rebased (multiple times) patch series by
Christian for allowing foreign BO imports for peer-to-peer buffer
access.

Amber Lin (1):
  drm/amdgpu: handle foreign BOs in the VM mapping

Christian König (5):
  drm: export drm_gem_prime_dmabuf_ops
  drm/amdgpu: disallow foreign BOs for UVD/VCE
  drm/amdgpu: disallow foreign BOs in the display path
  drm/amdgpu: separate BO from GEM object
  drm/amdgpu: enable foreign DMA-buf objects

Felix Kuehling (3):
  drm/amdgpu: implement vm_operations_struct.access
  drm/amdgpu: Enable SDMA context switching for CIK
  drm/amdgpu: Make SDMA phase quantum configurable

Jay Cornwall (1):
  drm/amdgpu: Send no-retry XNACK for all fault types

Yong Zhao (1):
  drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

shaoyunl (1):
  drm/amdgpu: Enable SDMA_CNTL.ATC_L1_ENABLE for SDMA on CZ

 drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  15 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |   7 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |   6 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |   6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     |  41 ++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c   |  79 ++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 147 +++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |   2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  17 +++-
 drivers/gpu/drm/amd/amdgpu/cik.c            |   9 +-
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c       |  60 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c    |   3 +
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c     |   3 +
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c      |  42 +++++++-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c      |  34 ++++++-
 drivers/gpu/drm/drm_prime.c                 |   3 +-
 include/drm/drmP.h                          |   2 +
 19 files changed, 446 insertions(+), 40 deletions(-)

-- 
1.9.1

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

[PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Felix Kuehling]

Allows gdb to access contents of user mode mapped BOs.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 130 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   2 +
 2 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 15148f1..3f927c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
 	man->size = size >> PAGE_SHIFT;
 }
 
+static struct vm_operations_struct amdgpu_ttm_vm_ops;
+static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
+						      * (appease checkpatch) */;
+static int amdgpu_ttm_bo_access_vram(struct amdgpu_bo *abo,
+				     unsigned long offset,
+				     void *buf, int len, int write)
+{
+	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
+	struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
+	uint32_t value = 0;
+	int result = 0;
+	uint64_t pos;
+	unsigned long flags;
+
+	while (offset >= (nodes->size << PAGE_SHIFT)) {
+		offset -= nodes->size << PAGE_SHIFT;
+		++nodes;
+	}
+	pos = (nodes->start << PAGE_SHIFT) + offset;
+
+	while (len && pos < adev->mc.mc_vram_size) {
+		uint64_t aligned_pos = pos & ~(uint64_t)3;
+		uint32_t bytes = 4 - (pos & 3);
+		uint32_t shift = (pos & 3) * 8;
+		uint32_t mask = 0xffffffff << shift;
+
+		if (len < bytes) {
+			mask &= 0xffffffff >> (bytes - len) * 8;
+			bytes = len;
+		}
+
+		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
+		WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
+		WREG32(mmMM_INDEX_HI, aligned_pos >> 31);
+		if (!write || mask != 0xffffffff)
+			value = RREG32(mmMM_DATA);
+		if (write) {
+			value &= ~mask;
+			value |= (*(uint32_t *)buf << shift) & mask;
+			WREG32(mmMM_DATA, value);
+		}
+		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
+		if (!write) {
+			value = (value & mask) >> shift;
+			memcpy(buf, &value, bytes);
+		}
+
+		result += bytes;
+		buf = (uint8_t *)buf + bytes;
+		pos += bytes;
+		len -= bytes;
+		if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) {
+			++nodes;
+			pos = (nodes->start << PAGE_SHIFT);
+		}
+	}
+
+	return result;
+}
+
+static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
+				     unsigned long offset,
+				     void *buf, int len, int write)
+{
+	struct ttm_buffer_object *bo = &abo->tbo;
+	struct ttm_bo_kmap_obj map;
+	void *ptr;
+	bool is_iomem;
+	int r;
+
+	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
+	if (r)
+		return r;
+	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
+	WARN_ON(is_iomem);
+	if (write)
+		memcpy(ptr, buf, len);
+	else
+		memcpy(buf, ptr, len);
+	ttm_bo_kunmap(&map);
+
+	return len;
+}
+
+static int amdgpu_ttm_vm_access(struct vm_area_struct *vma, unsigned long addr,
+				void *buf, int len, int write)
+{
+	unsigned long offset = (addr) - vma->vm_start;
+	struct ttm_buffer_object *bo = vma->vm_private_data;
+	struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
+	unsigned domain;
+	int result;
+
+	result = amdgpu_bo_reserve(abo, false);
+	if (result != 0)
+		return result;
+
+	domain = amdgpu_mem_type_to_domain(bo->mem.mem_type);
+	if (domain == AMDGPU_GEM_DOMAIN_VRAM)
+		result = amdgpu_ttm_bo_access_vram(abo, offset,
+						   buf, len, write);
+	else
+		result = amdgpu_ttm_bo_access_kmap(abo, offset,
+						   buf, len, write);
+	amdgpu_bo_unreserve(abo);
+
+	return len;
+}
+
+int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
+		   struct ttm_bo_device *bdev)
+{
+	int r;
+
+	r = ttm_bo_mmap(filp, vma, bdev);
+	if (unlikely(r != 0))
+		return r;
+
+	if (unlikely(ttm_vm_ops == NULL)) {
+		ttm_vm_ops = vma->vm_ops;
+		amdgpu_ttm_vm_ops = *ttm_vm_ops;
+		amdgpu_ttm_vm_ops.access = &amdgpu_ttm_vm_access;
+	}
+	vma->vm_ops = &amdgpu_ttm_vm_ops;
+
+	return 0;
+}
+
 int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
 {
 	struct drm_file *file_priv;
@@ -1250,7 +1378,7 @@ int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
 	if (adev == NULL)
 		return -EINVAL;
 
-	return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
+	return amdgpu_bo_mmap(filp, vma, &adev->mman.bdev);
 }
 
 int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index 776a20a..1eb605c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -73,6 +73,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
 			struct dma_fence **fence);
 
 int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
+int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
+		   struct ttm_bo_device *bdev);
 bool amdgpu_ttm_is_bound(struct ttm_tt *ttm);
 int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct ttm_mem_reg *bo_mem);
 
-- 
1.9.1

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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Felix Kuehling]

Any comments on this one?

This was requested by the HSA runtime team a long time ago as a
debugging feature. It allows gdb to access the content of CPU-mapped
BOs. I imagine this may be useful for user mode driver developers.

Does anyone dare to give me a Reviewed-by?

Regards,
  Felix


On 17-07-03 05:11 PM, Felix Kuehling wrote:
> Allows gdb to access contents of user mode mapped BOs.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 130 +++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   2 +
>  2 files changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 15148f1..3f927c2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
>  	man->size = size >> PAGE_SHIFT;
>  }
>  
> +static struct vm_operations_struct amdgpu_ttm_vm_ops;
> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
> +						      * (appease checkpatch) */;
> +static int amdgpu_ttm_bo_access_vram(struct amdgpu_bo *abo,
> +				     unsigned long offset,
> +				     void *buf, int len, int write)
> +{
> +	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
> +	struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
> +	uint32_t value = 0;
> +	int result = 0;
> +	uint64_t pos;
> +	unsigned long flags;
> +
> +	while (offset >= (nodes->size << PAGE_SHIFT)) {
> +		offset -= nodes->size << PAGE_SHIFT;
> +		++nodes;
> +	}
> +	pos = (nodes->start << PAGE_SHIFT) + offset;
> +
> +	while (len && pos < adev->mc.mc_vram_size) {
> +		uint64_t aligned_pos = pos & ~(uint64_t)3;
> +		uint32_t bytes = 4 - (pos & 3);
> +		uint32_t shift = (pos & 3) * 8;
> +		uint32_t mask = 0xffffffff << shift;
> +
> +		if (len < bytes) {
> +			mask &= 0xffffffff >> (bytes - len) * 8;
> +			bytes = len;
> +		}
> +
> +		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> +		WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
> +		WREG32(mmMM_INDEX_HI, aligned_pos >> 31);
> +		if (!write || mask != 0xffffffff)
> +			value = RREG32(mmMM_DATA);
> +		if (write) {
> +			value &= ~mask;
> +			value |= (*(uint32_t *)buf << shift) & mask;
> +			WREG32(mmMM_DATA, value);
> +		}
> +		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> +		if (!write) {
> +			value = (value & mask) >> shift;
> +			memcpy(buf, &value, bytes);
> +		}
> +
> +		result += bytes;
> +		buf = (uint8_t *)buf + bytes;
> +		pos += bytes;
> +		len -= bytes;
> +		if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) {
> +			++nodes;
> +			pos = (nodes->start << PAGE_SHIFT);
> +		}
> +	}
> +
> +	return result;
> +}
> +
> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
> +				     unsigned long offset,
> +				     void *buf, int len, int write)
> +{
> +	struct ttm_buffer_object *bo = &abo->tbo;
> +	struct ttm_bo_kmap_obj map;
> +	void *ptr;
> +	bool is_iomem;
> +	int r;
> +
> +	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
> +	if (r)
> +		return r;
> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
> +	WARN_ON(is_iomem);
> +	if (write)
> +		memcpy(ptr, buf, len);
> +	else
> +		memcpy(buf, ptr, len);
> +	ttm_bo_kunmap(&map);
> +
> +	return len;
> +}
> +
> +static int amdgpu_ttm_vm_access(struct vm_area_struct *vma, unsigned long addr,
> +				void *buf, int len, int write)
> +{
> +	unsigned long offset = (addr) - vma->vm_start;
> +	struct ttm_buffer_object *bo = vma->vm_private_data;
> +	struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
> +	unsigned domain;
> +	int result;
> +
> +	result = amdgpu_bo_reserve(abo, false);
> +	if (result != 0)
> +		return result;
> +
> +	domain = amdgpu_mem_type_to_domain(bo->mem.mem_type);
> +	if (domain == AMDGPU_GEM_DOMAIN_VRAM)
> +		result = amdgpu_ttm_bo_access_vram(abo, offset,
> +						   buf, len, write);
> +	else
> +		result = amdgpu_ttm_bo_access_kmap(abo, offset,
> +						   buf, len, write);
> +	amdgpu_bo_unreserve(abo);
> +
> +	return len;
> +}
> +
> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> +		   struct ttm_bo_device *bdev)
> +{
> +	int r;
> +
> +	r = ttm_bo_mmap(filp, vma, bdev);
> +	if (unlikely(r != 0))
> +		return r;
> +
> +	if (unlikely(ttm_vm_ops == NULL)) {
> +		ttm_vm_ops = vma->vm_ops;
> +		amdgpu_ttm_vm_ops = *ttm_vm_ops;
> +		amdgpu_ttm_vm_ops.access = &amdgpu_ttm_vm_access;
> +	}
> +	vma->vm_ops = &amdgpu_ttm_vm_ops;
> +
> +	return 0;
> +}
> +
>  int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
>  	struct drm_file *file_priv;
> @@ -1250,7 +1378,7 @@ int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>  	if (adev == NULL)
>  		return -EINVAL;
>  
> -	return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
> +	return amdgpu_bo_mmap(filp, vma, &adev->mman.bdev);
>  }
>  
>  int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index 776a20a..1eb605c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -73,6 +73,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>  			struct dma_fence **fence);
>  
>  int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
> +		   struct ttm_bo_device *bdev);
>  bool amdgpu_ttm_is_bound(struct ttm_tt *ttm);
>  int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct ttm_mem_reg *bo_mem);
>  

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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[axie]

Hi Felix,

I think you might want to add a boundary check to limit the ptrace 
access inside the BO.

Otherwise there may be a potential security leak allowing others to 
access the whole memory by using ptrace system call.

-Alex Bin


On 2017-07-12 01:37 AM, Felix Kuehling wrote:
> Any comments on this one?
>
> This was requested by the HSA runtime team a long time ago as a
> debugging feature. It allows gdb to access the content of CPU-mapped
> BOs. I imagine this may be useful for user mode driver developers.
>
> Does anyone dare to give me a Reviewed-by?
>
> Regards,
>    Felix
>
>
> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>> Allows gdb to access contents of user mode mapped BOs.
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 130 +++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   2 +
>>   2 files changed, 131 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 15148f1..3f927c2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
>>   	man->size = size >> PAGE_SHIFT;
>>   }
>>   
>> +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>> +						      * (appease checkpatch) */;
>> +static int amdgpu_ttm_bo_access_vram(struct amdgpu_bo *abo,
>> +				     unsigned long offset,
>> +				     void *buf, int len, int write)
>> +{
>> +	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> +	struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
>> +	uint32_t value = 0;
>> +	int result = 0;
>> +	uint64_t pos;
>> +	unsigned long flags;
>> +
>> +	while (offset >= (nodes->size << PAGE_SHIFT)) {
>> +		offset -= nodes->size << PAGE_SHIFT;
>> +		++nodes;
>> +	}
>> +	pos = (nodes->start << PAGE_SHIFT) + offset;
>> +
>> +	while (len && pos < adev->mc.mc_vram_size) {
>> +		uint64_t aligned_pos = pos & ~(uint64_t)3;
>> +		uint32_t bytes = 4 - (pos & 3);
>> +		uint32_t shift = (pos & 3) * 8;
>> +		uint32_t mask = 0xffffffff << shift;
>> +
>> +		if (len < bytes) {
>> +			mask &= 0xffffffff >> (bytes - len) * 8;
>> +			bytes = len;
>> +		}
>> +
>> +		spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>> +		WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
>> +		WREG32(mmMM_INDEX_HI, aligned_pos >> 31);
>> +		if (!write || mask != 0xffffffff)
>> +			value = RREG32(mmMM_DATA);
>> +		if (write) {
>> +			value &= ~mask;
>> +			value |= (*(uint32_t *)buf << shift) & mask;
>> +			WREG32(mmMM_DATA, value);
>> +		}
>> +		spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>> +		if (!write) {
>> +			value = (value & mask) >> shift;
>> +			memcpy(buf, &value, bytes);
>> +		}
>> +
>> +		result += bytes;
>> +		buf = (uint8_t *)buf + bytes;
>> +		pos += bytes;
>> +		len -= bytes;
>> +		if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) {
>> +			++nodes;
>> +			pos = (nodes->start << PAGE_SHIFT);
>> +		}
>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>> +				     unsigned long offset,
>> +				     void *buf, int len, int write)
>> +{
>> +	struct ttm_buffer_object *bo = &abo->tbo;
>> +	struct ttm_bo_kmap_obj map;
>> +	void *ptr;
>> +	bool is_iomem;
>> +	int r;
>> +
>> +	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>> +	if (r)
>> +		return r;
>> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>> +	WARN_ON(is_iomem);
>> +	if (write)
>> +		memcpy(ptr, buf, len);
>> +	else
>> +		memcpy(buf, ptr, len);
>> +	ttm_bo_kunmap(&map);
>> +
>> +	return len;
>> +}
>> +
>> +static int amdgpu_ttm_vm_access(struct vm_area_struct *vma, unsigned long addr,
>> +				void *buf, int len, int write)
>> +{
>> +	unsigned long offset = (addr) - vma->vm_start;
>> +	struct ttm_buffer_object *bo = vma->vm_private_data;
>> +	struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
>> +	unsigned domain;
>> +	int result;
>> +
>> +	result = amdgpu_bo_reserve(abo, false);
>> +	if (result != 0)
>> +		return result;
>> +
>> +	domain = amdgpu_mem_type_to_domain(bo->mem.mem_type);
>> +	if (domain == AMDGPU_GEM_DOMAIN_VRAM)
>> +		result = amdgpu_ttm_bo_access_vram(abo, offset,
>> +						   buf, len, write);
>> +	else
>> +		result = amdgpu_ttm_bo_access_kmap(abo, offset,
>> +						   buf, len, write);
>> +	amdgpu_bo_unreserve(abo);
>> +
>> +	return len;
>> +}
>> +
>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>> +		   struct ttm_bo_device *bdev)
>> +{
>> +	int r;
>> +
>> +	r = ttm_bo_mmap(filp, vma, bdev);
>> +	if (unlikely(r != 0))
>> +		return r;
>> +
>> +	if (unlikely(ttm_vm_ops == NULL)) {
>> +		ttm_vm_ops = vma->vm_ops;
>> +		amdgpu_ttm_vm_ops = *ttm_vm_ops;
>> +		amdgpu_ttm_vm_ops.access = &amdgpu_ttm_vm_access;
>> +	}
>> +	vma->vm_ops = &amdgpu_ttm_vm_ops;
>> +
>> +	return 0;
>> +}
>> +
>>   int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>>   {
>>   	struct drm_file *file_priv;
>> @@ -1250,7 +1378,7 @@ int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>>   	if (adev == NULL)
>>   		return -EINVAL;
>>   
>> -	return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
>> +	return amdgpu_bo_mmap(filp, vma, &adev->mman.bdev);
>>   }
>>   
>>   int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index 776a20a..1eb605c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -73,6 +73,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>   			struct dma_fence **fence);
>>   
>>   int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>> +		   struct ttm_bo_device *bdev);
>>   bool amdgpu_ttm_is_bound(struct ttm_tt *ttm);
>>   int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct ttm_mem_reg *bo_mem);
>>   
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Christian König]

Hi Felix,

in theory all ranges should be limited by the VMA, the TTM mapping code 
makes that assumption as well.

But I agree that we better be save than sorry.

Additional to that one thing which stand out:
> +    r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);

Please try to avoid mapping the whole BO if you want to access only some 
bytes. That in turn usually result in changing the page tables which is 
a bit invasive for the ptrace callback.

In general we should try to avoid accessing more than one page at once.

Regards,
Christian.

Am 12.07.2017 um 08:26 schrieb axie:
> Hi Felix,
>
> I think you might want to add a boundary check to limit the ptrace 
> access inside the BO.
>
> Otherwise there may be a potential security leak allowing others to 
> access the whole memory by using ptrace system call.
>
> -Alex Bin
>
>
> On 2017-07-12 01:37 AM, Felix Kuehling wrote:
>> Any comments on this one?
>>
>> This was requested by the HSA runtime team a long time ago as a
>> debugging feature. It allows gdb to access the content of CPU-mapped
>> BOs. I imagine this may be useful for user mode driver developers.
>>
>> Does anyone dare to give me a Reviewed-by?
>>
>> Regards,
>>    Felix
>>
>>
>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>> Allows gdb to access contents of user mode mapped BOs.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 130 
>>> +++++++++++++++++++++++++++++++-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   2 +
>>>   2 files changed, 131 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 15148f1..3f927c2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct 
>>> amdgpu_device *adev, u64 size)
>>>       man->size = size >> PAGE_SHIFT;
>>>   }
>>>   +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>>> +                              * (appease checkpatch) */;
>>> +static int amdgpu_ttm_bo_access_vram(struct amdgpu_bo *abo,
>>> +                     unsigned long offset,
>>> +                     void *buf, int len, int write)
>>> +{
>>> +    struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>>> +    struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
>>> +    uint32_t value = 0;
>>> +    int result = 0;
>>> +    uint64_t pos;
>>> +    unsigned long flags;
>>> +
>>> +    while (offset >= (nodes->size << PAGE_SHIFT)) {
>>> +        offset -= nodes->size << PAGE_SHIFT;
>>> +        ++nodes;
>>> +    }
>>> +    pos = (nodes->start << PAGE_SHIFT) + offset;
>>> +
>>> +    while (len && pos < adev->mc.mc_vram_size) {
>>> +        uint64_t aligned_pos = pos & ~(uint64_t)3;
>>> +        uint32_t bytes = 4 - (pos & 3);
>>> +        uint32_t shift = (pos & 3) * 8;
>>> +        uint32_t mask = 0xffffffff << shift;
>>> +
>>> +        if (len < bytes) {
>>> +            mask &= 0xffffffff >> (bytes - len) * 8;
>>> +            bytes = len;
>>> +        }
>>> +
>>> +        spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>> +        WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
>>> +        WREG32(mmMM_INDEX_HI, aligned_pos >> 31);
>>> +        if (!write || mask != 0xffffffff)
>>> +            value = RREG32(mmMM_DATA);
>>> +        if (write) {
>>> +            value &= ~mask;
>>> +            value |= (*(uint32_t *)buf << shift) & mask;
>>> +            WREG32(mmMM_DATA, value);
>>> +        }
>>> +        spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>> +        if (!write) {
>>> +            value = (value & mask) >> shift;
>>> +            memcpy(buf, &value, bytes);
>>> +        }
>>> +
>>> +        result += bytes;
>>> +        buf = (uint8_t *)buf + bytes;
>>> +        pos += bytes;
>>> +        len -= bytes;
>>> +        if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) {
>>> +            ++nodes;
>>> +            pos = (nodes->start << PAGE_SHIFT);
>>> +        }
>>> +    }
>>> +
>>> +    return result;
>>> +}
>>> +
>>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>>> +                     unsigned long offset,
>>> +                     void *buf, int len, int write)
>>> +{
>>> +    struct ttm_buffer_object *bo = &abo->tbo;
>>> +    struct ttm_bo_kmap_obj map;
>>> +    void *ptr;
>>> +    bool is_iomem;
>>> +    int r;
>>> +
>>> +    r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>>> +    if (r)
>>> +        return r;
>>> +    ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>> +    WARN_ON(is_iomem);
>>> +    if (write)
>>> +        memcpy(ptr, buf, len);
>>> +    else
>>> +        memcpy(buf, ptr, len);
>>> +    ttm_bo_kunmap(&map);
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +static int amdgpu_ttm_vm_access(struct vm_area_struct *vma, 
>>> unsigned long addr,
>>> +                void *buf, int len, int write)
>>> +{
>>> +    unsigned long offset = (addr) - vma->vm_start;
>>> +    struct ttm_buffer_object *bo = vma->vm_private_data;
>>> +    struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
>>> +    unsigned domain;
>>> +    int result;
>>> +
>>> +    result = amdgpu_bo_reserve(abo, false);
>>> +    if (result != 0)
>>> +        return result;
>>> +
>>> +    domain = amdgpu_mem_type_to_domain(bo->mem.mem_type);
>>> +    if (domain == AMDGPU_GEM_DOMAIN_VRAM)
>>> +        result = amdgpu_ttm_bo_access_vram(abo, offset,
>>> +                           buf, len, write);
>>> +    else
>>> +        result = amdgpu_ttm_bo_access_kmap(abo, offset,
>>> +                           buf, len, write);
>>> +    amdgpu_bo_unreserve(abo);
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>>> +           struct ttm_bo_device *bdev)
>>> +{
>>> +    int r;
>>> +
>>> +    r = ttm_bo_mmap(filp, vma, bdev);
>>> +    if (unlikely(r != 0))
>>> +        return r;
>>> +
>>> +    if (unlikely(ttm_vm_ops == NULL)) {
>>> +        ttm_vm_ops = vma->vm_ops;
>>> +        amdgpu_ttm_vm_ops = *ttm_vm_ops;
>>> +        amdgpu_ttm_vm_ops.access = &amdgpu_ttm_vm_access;
>>> +    }
>>> +    vma->vm_ops = &amdgpu_ttm_vm_ops;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>>>   {
>>>       struct drm_file *file_priv;
>>> @@ -1250,7 +1378,7 @@ int amdgpu_mmap(struct file *filp, struct 
>>> vm_area_struct *vma)
>>>       if (adev == NULL)
>>>           return -EINVAL;
>>>   -    return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
>>> +    return amdgpu_bo_mmap(filp, vma, &adev->mman.bdev);
>>>   }
>>>     int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t 
>>> src_offset,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> index 776a20a..1eb605c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> @@ -73,6 +73,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>>               struct dma_fence **fence);
>>>     int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
>>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>>> +           struct ttm_bo_device *bdev);
>>>   bool amdgpu_ttm_is_bound(struct ttm_tt *ttm);
>>>   int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct 
>>> ttm_mem_reg *bo_mem);
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[axie]

I agree, in theory, all ranges should be limited by the VMA. But in 
reality, I did not see such boundary check yesterday when I quickly read 
through the source codes where ptrace calls access function.

We may fix it in ptrace if that is really missing. For the time being, 
adding a boundary check in AMDGPU side may be a good idea.

Thanks,
Alex Bin

On 2017-07-12 03:22 AM, Christian König wrote:
> Hi Felix,
>
> in theory all ranges should be limited by the VMA, the TTM mapping 
> code makes that assumption as well.
>
> But I agree that we better be save than sorry.
>
> Additional to that one thing which stand out:
>> +    r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>
> Please try to avoid mapping the whole BO if you want to access only 
> some bytes. That in turn usually result in changing the page tables 
> which is a bit invasive for the ptrace callback.
>
> In general we should try to avoid accessing more than one page at once.
>
> Regards,
> Christian.
>
> Am 12.07.2017 um 08:26 schrieb axie:
>> Hi Felix,
>>
>> I think you might want to add a boundary check to limit the ptrace 
>> access inside the BO.
>>
>> Otherwise there may be a potential security leak allowing others to 
>> access the whole memory by using ptrace system call.
>>
>> -Alex Bin
>>
>>
>> On 2017-07-12 01:37 AM, Felix Kuehling wrote:
>>> Any comments on this one?
>>>
>>> This was requested by the HSA runtime team a long time ago as a
>>> debugging feature. It allows gdb to access the content of CPU-mapped
>>> BOs. I imagine this may be useful for user mode driver developers.
>>>
>>> Does anyone dare to give me a Reviewed-by?
>>>
>>> Regards,
>>>    Felix
>>>
>>>
>>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>>> Allows gdb to access contents of user mode mapped BOs.
>>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 130 
>>>> +++++++++++++++++++++++++++++++-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   2 +
>>>>   2 files changed, 131 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 15148f1..3f927c2 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct 
>>>> amdgpu_device *adev, u64 size)
>>>>       man->size = size >> PAGE_SHIFT;
>>>>   }
>>>>   +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>>>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>>>> +                              * (appease checkpatch) */;
>>>> +static int amdgpu_ttm_bo_access_vram(struct amdgpu_bo *abo,
>>>> +                     unsigned long offset,
>>>> +                     void *buf, int len, int write)
>>>> +{
>>>> +    struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>>>> +    struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
>>>> +    uint32_t value = 0;
>>>> +    int result = 0;
>>>> +    uint64_t pos;
>>>> +    unsigned long flags;
>>>> +
>>>> +    while (offset >= (nodes->size << PAGE_SHIFT)) {
>>>> +        offset -= nodes->size << PAGE_SHIFT;
>>>> +        ++nodes;
>>>> +    }
>>>> +    pos = (nodes->start << PAGE_SHIFT) + offset;
>>>> +
>>>> +    while (len && pos < adev->mc.mc_vram_size) {
>>>> +        uint64_t aligned_pos = pos & ~(uint64_t)3;
>>>> +        uint32_t bytes = 4 - (pos & 3);
>>>> +        uint32_t shift = (pos & 3) * 8;
>>>> +        uint32_t mask = 0xffffffff << shift;
>>>> +
>>>> +        if (len < bytes) {
>>>> +            mask &= 0xffffffff >> (bytes - len) * 8;
>>>> +            bytes = len;
>>>> +        }
>>>> +
>>>> +        spin_lock_irqsave(&adev->mmio_idx_lock, flags);
>>>> +        WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x80000000);
>>>> +        WREG32(mmMM_INDEX_HI, aligned_pos >> 31);
>>>> +        if (!write || mask != 0xffffffff)
>>>> +            value = RREG32(mmMM_DATA);
>>>> +        if (write) {
>>>> +            value &= ~mask;
>>>> +            value |= (*(uint32_t *)buf << shift) & mask;
>>>> +            WREG32(mmMM_DATA, value);
>>>> +        }
>>>> +        spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
>>>> +        if (!write) {
>>>> +            value = (value & mask) >> shift;
>>>> +            memcpy(buf, &value, bytes);
>>>> +        }
>>>> +
>>>> +        result += bytes;
>>>> +        buf = (uint8_t *)buf + bytes;
>>>> +        pos += bytes;
>>>> +        len -= bytes;
>>>> +        if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) {
>>>> +            ++nodes;
>>>> +            pos = (nodes->start << PAGE_SHIFT);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return result;
>>>> +}
>>>> +
>>>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>>>> +                     unsigned long offset,
>>>> +                     void *buf, int len, int write)
>>>> +{
>>>> +    struct ttm_buffer_object *bo = &abo->tbo;
>>>> +    struct ttm_bo_kmap_obj map;
>>>> +    void *ptr;
>>>> +    bool is_iomem;
>>>> +    int r;
>>>> +
>>>> +    r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>>>> +    if (r)
>>>> +        return r;
>>>> +    ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>>> +    WARN_ON(is_iomem);
>>>> +    if (write)
>>>> +        memcpy(ptr, buf, len);
>>>> +    else
>>>> +        memcpy(buf, ptr, len);
>>>> +    ttm_bo_kunmap(&map);
>>>> +
>>>> +    return len;
>>>> +}
>>>> +
>>>> +static int amdgpu_ttm_vm_access(struct vm_area_struct *vma, 
>>>> unsigned long addr,
>>>> +                void *buf, int len, int write)
>>>> +{
>>>> +    unsigned long offset = (addr) - vma->vm_start;
>>>> +    struct ttm_buffer_object *bo = vma->vm_private_data;
>>>> +    struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
>>>> +    unsigned domain;
>>>> +    int result;
>>>> +
>>>> +    result = amdgpu_bo_reserve(abo, false);
>>>> +    if (result != 0)
>>>> +        return result;
>>>> +
>>>> +    domain = amdgpu_mem_type_to_domain(bo->mem.mem_type);
>>>> +    if (domain == AMDGPU_GEM_DOMAIN_VRAM)
>>>> +        result = amdgpu_ttm_bo_access_vram(abo, offset,
>>>> +                           buf, len, write);
>>>> +    else
>>>> +        result = amdgpu_ttm_bo_access_kmap(abo, offset,
>>>> +                           buf, len, write);
>>>> +    amdgpu_bo_unreserve(abo);
>>>> +
>>>> +    return len;
>>>> +}
>>>> +
>>>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>>>> +           struct ttm_bo_device *bdev)
>>>> +{
>>>> +    int r;
>>>> +
>>>> +    r = ttm_bo_mmap(filp, vma, bdev);
>>>> +    if (unlikely(r != 0))
>>>> +        return r;
>>>> +
>>>> +    if (unlikely(ttm_vm_ops == NULL)) {
>>>> +        ttm_vm_ops = vma->vm_ops;
>>>> +        amdgpu_ttm_vm_ops = *ttm_vm_ops;
>>>> +        amdgpu_ttm_vm_ops.access = &amdgpu_ttm_vm_access;
>>>> +    }
>>>> +    vma->vm_ops = &amdgpu_ttm_vm_ops;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
>>>>   {
>>>>       struct drm_file *file_priv;
>>>> @@ -1250,7 +1378,7 @@ int amdgpu_mmap(struct file *filp, struct 
>>>> vm_area_struct *vma)
>>>>       if (adev == NULL)
>>>>           return -EINVAL;
>>>>   -    return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
>>>> +    return amdgpu_bo_mmap(filp, vma, &adev->mman.bdev);
>>>>   }
>>>>     int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t 
>>>> src_offset,
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> index 776a20a..1eb605c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> @@ -73,6 +73,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>>>               struct dma_fence **fence);
>>>>     int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma);
>>>> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma,
>>>> +           struct ttm_bo_device *bdev);
>>>>   bool amdgpu_ttm_is_bound(struct ttm_tt *ttm);
>>>>   int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct 
>>>> ttm_mem_reg *bo_mem);
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>

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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Michel Dänzer]

On 12/07/17 02:37 PM, Felix Kuehling wrote:
> Any comments on this one?
> 
> This was requested by the HSA runtime team a long time ago as a
> debugging feature. It allows gdb to access the content of CPU-mapped
> BOs. I imagine this may be useful for user mode driver developers.

Adding the dri-devel list.


>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 15148f1..3f927c2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
>>  	man->size = size >> PAGE_SHIFT;
>>  }
>>  
>> +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>> +						      * (appease checkpatch) */;

How does this appease checkpatch?


>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>> +				     unsigned long offset,
>> +				     void *buf, int len, int write)
>> +{
>> +	struct ttm_buffer_object *bo = &abo->tbo;
>> +	struct ttm_bo_kmap_obj map;
>> +	void *ptr;
>> +	bool is_iomem;
>> +	int r;
>> +
>> +	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>> +	if (r)
>> +		return r;
>> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>> +	WARN_ON(is_iomem);
>> +	if (write)
>> +		memcpy(ptr, buf, len);
>> +	else
>> +		memcpy(buf, ptr, len);
>> +	ttm_bo_kunmap(&map);
>> +
>> +	return len;
>> +}

This could be in TTM, right? As a helper function and/or a generic
vm_operations_struct::access hook (at least for GTT/CPU domains).


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Felix Kuehling]

On 17-07-12 04:01 AM, Michel Dänzer wrote:
> On 12/07/17 02:37 PM, Felix Kuehling wrote:
>> Any comments on this one?
>>
>> This was requested by the HSA runtime team a long time ago as a
>> debugging feature. It allows gdb to access the content of CPU-mapped
>> BOs. I imagine this may be useful for user mode driver developers.
> Adding the dri-devel list.
>
>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 15148f1..3f927c2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
>>>  	man->size = size >> PAGE_SHIFT;
>>>  }
>>>  
>>> +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>>> +						      * (appease checkpatch) */;
> How does this appease checkpatch?

Checkpatch doesn't like explicit initialization of global variables to
0. Uninitialized data is automatically 0, so no point wasting space in
the initialized data segment.

>
>
>>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>>> +				     unsigned long offset,
>>> +				     void *buf, int len, int write)
>>> +{
>>> +	struct ttm_buffer_object *bo = &abo->tbo;
>>> +	struct ttm_bo_kmap_obj map;
>>> +	void *ptr;
>>> +	bool is_iomem;
>>> +	int r;
>>> +
>>> +	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>>> +	if (r)
>>> +		return r;
>>> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>> +	WARN_ON(is_iomem);
>>> +	if (write)
>>> +		memcpy(ptr, buf, len);
>>> +	else
>>> +		memcpy(buf, ptr, len);
>>> +	ttm_bo_kunmap(&map);
>>> +
>>> +	return len;
>>> +}
> This could be in TTM, right? As a helper function and/or a generic
> vm_operations_struct::access hook (at least for GTT/CPU domains).

I guess. If I were to get TTM involved, I'd also have TTM install its
own access hook in the vm_ops, so the driver doesn't need to override
it. Then I'd add driver-specific callbacks in ttm_bo_driver for
accessing VRAM and GTT/CPU memory. TTM could offer something like
amdgpu_ttm_bo_access_kmap as a helper for use by all drivers.

Regards,
  Felix

>
>

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

Re: [PATCH 01/12] drm/amdgpu: implement vm_operations_struct.access

[Michel Dänzer]

On 13/07/17 02:27 AM, Felix Kuehling wrote:
> On 17-07-12 04:01 AM, Michel Dänzer wrote:
>> On 12/07/17 02:37 PM, Felix Kuehling wrote:
>>> Any comments on this one?
>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 15148f1..3f927c2 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -1237,6 +1237,134 @@ void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size)
>>>>  	man->size = size >> PAGE_SHIFT;
>>>>  }
>>>>  
>>>> +static struct vm_operations_struct amdgpu_ttm_vm_ops;
>>>> +static const struct vm_operations_struct *ttm_vm_ops /* = NULL;
>>>> +						      * (appease checkpatch) */;
>> How does this appease checkpatch?
> 
> Checkpatch doesn't like explicit initialization of global variables to
> 0. Uninitialized data is automatically 0, so no point wasting space in
> the initialized data segment.

That is true for static variables, so I suspect checkpatch explicitly
complains about initializing those to 0. Just remove the comment (and
the initialization).


>>>> +static int amdgpu_ttm_bo_access_kmap(struct amdgpu_bo *abo,
>>>> +				     unsigned long offset,
>>>> +				     void *buf, int len, int write)
>>>> +{
>>>> +	struct ttm_buffer_object *bo = &abo->tbo;
>>>> +	struct ttm_bo_kmap_obj map;
>>>> +	void *ptr;
>>>> +	bool is_iomem;
>>>> +	int r;
>>>> +
>>>> +	r = ttm_bo_kmap(bo, 0, bo->num_pages, &map);
>>>> +	if (r)
>>>> +		return r;
>>>> +	ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset;
>>>> +	WARN_ON(is_iomem);
>>>> +	if (write)
>>>> +		memcpy(ptr, buf, len);
>>>> +	else
>>>> +		memcpy(buf, ptr, len);
>>>> +	ttm_bo_kunmap(&map);
>>>> +
>>>> +	return len;
>>>> +}
>> This could be in TTM, right? As a helper function and/or a generic
>> vm_operations_struct::access hook (at least for GTT/CPU domains).
> 
> I guess. If I were to get TTM involved, I'd also have TTM install its
> own access hook in the vm_ops, so the driver doesn't need to override
> it. Then I'd add driver-specific callbacks in ttm_bo_driver for
> accessing VRAM

So far, I was thinking basically the same thing.


> and GTT/CPU memory. TTM could offer something like
> amdgpu_ttm_bo_access_kmap as a helper for use by all drivers.

But this doesn't seem necessary, since GTT/CPU domain CPU access should
work the same independent of the driver.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 02/12] drm/amdgpu: Enable SDMA_CNTL.ATC_L1_ENABLE for SDMA on CZ

[Felix Kuehling]

From: shaoyunl <Shaoyun.Liu@amd.com>

For GFX context, the  ATC bit in SDMA*_GFX_VIRTUAL_ADDRESS  can be cleared
to perform in VM mode. For RLC context, to support ATC mode , ATC bit in
SDMA*_RLC*_VIRTUAL_ADDRESS should be set. SDMA_CNTL.ATC_L1_ENABLE bit is
global setting that enables the  L1-L2 translation for ATC address.

Signed-off-by: shaoyun liu <shaoyun.liu@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index 1d766ae..67a29fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -556,12 +556,18 @@ static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
-		if (enable)
+		if (enable) {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 1);
-		else
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					ATC_L1_ENABLE, 1);
+		} else {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 0);
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					ATC_L1_ENABLE, 1);
+		}
+
 		WREG32(mmSDMA0_CNTL + sdma_offsets[i], f32_cntl);
 	}
 }
-- 
1.9.1

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

Re: [PATCH 02/12] drm/amdgpu: Enable SDMA_CNTL.ATC_L1_ENABLE for SDMA on CZ

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: shaoyunl <Shaoyun.Liu@amd.com>
>
> For GFX context, the  ATC bit in SDMA*_GFX_VIRTUAL_ADDRESS  can be cleared
> to perform in VM mode. For RLC context, to support ATC mode , ATC bit in
> SDMA*_RLC*_VIRTUAL_ADDRESS should be set. SDMA_CNTL.ATC_L1_ENABLE bit is
> global setting that enables the  L1-L2 translation for ATC address.
>
> Signed-off-by: shaoyun liu <shaoyun.liu@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

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

> ---
>  drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> index 1d766ae..67a29fb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> @@ -556,12 +556,18 @@ static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
>
>         for (i = 0; i < adev->sdma.num_instances; i++) {
>                 f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
> -               if (enable)
> +               if (enable) {
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         AUTO_CTXSW_ENABLE, 1);
> -               else
> +                       f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
> +                                       ATC_L1_ENABLE, 1);
> +               } else {
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         AUTO_CTXSW_ENABLE, 0);
> +                       f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
> +                                       ATC_L1_ENABLE, 1);
> +               }
> +
>                 WREG32(mmSDMA0_CNTL + sdma_offsets[i], f32_cntl);
>         }
>  }
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 03/12] drm/amdgpu: Enable SDMA context switching for CIK

[Felix Kuehling]

Enable SDMA context switching on CIK (copied from sdma_v3_0.c).

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index c216e16..4a9cea0 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -342,6 +342,33 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)
 }
 
 /**
+ * cik_ctx_switch_enable - stop the async dma engines context switch
+ *
+ * @adev: amdgpu_device pointer
+ * @enable: enable/disable the DMA MEs context switch.
+ *
+ * Halt or unhalt the async dma engines context switch (VI).
+ */
+static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
+{
+	u32 f32_cntl;
+	int i;
+
+	for (i = 0; i < adev->sdma.num_instances; i++) {
+		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
+		if (enable) {
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					AUTO_CTXSW_ENABLE, 1);
+		} else {
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					AUTO_CTXSW_ENABLE, 0);
+		}
+
+		WREG32(mmSDMA0_CNTL + sdma_offsets[i], f32_cntl);
+	}
+}
+
+/**
  * cik_sdma_enable - stop the async dma engines
  *
  * @adev: amdgpu_device pointer
@@ -537,6 +564,8 @@ static int cik_sdma_start(struct amdgpu_device *adev)
 
 	/* halt the engine before programing */
 	cik_sdma_enable(adev, false);
+	/* enable sdma ring preemption */
+	cik_ctx_switch_enable(adev, true);
 
 	/* start the gfx rings and rlc compute queues */
 	r = cik_sdma_gfx_resume(adev);
@@ -984,6 +1013,7 @@ static int cik_sdma_hw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	cik_ctx_switch_enable(adev, false);
 	cik_sdma_enable(adev, false);
 
 	return 0;
-- 
1.9.1

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

RE: [PATCH 03/12] drm/amdgpu: Enable SDMA context switching for CIK

[Zhou, David(ChunMing)]

Reviewed-by: Chunming Zhou <david1.zhou@amd.com>

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Felix Kuehling
Sent: Tuesday, July 04, 2017 5:11 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix <Felix.Kuehling@amd.com>
Subject: [PATCH 03/12] drm/amdgpu: Enable SDMA context switching for CIK

Enable SDMA context switching on CIK (copied from sdma_v3_0.c).

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index c216e16..4a9cea0 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -342,6 +342,33 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)  }
 
 /**
+ * cik_ctx_switch_enable - stop the async dma engines context switch
+ *
+ * @adev: amdgpu_device pointer
+ * @enable: enable/disable the DMA MEs context switch.
+ *
+ * Halt or unhalt the async dma engines context switch (VI).
+ */
+static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool 
+enable) {
+	u32 f32_cntl;
+	int i;
+
+	for (i = 0; i < adev->sdma.num_instances; i++) {
+		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
+		if (enable) {
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					AUTO_CTXSW_ENABLE, 1);
+		} else {
+			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
+					AUTO_CTXSW_ENABLE, 0);
+		}
+
+		WREG32(mmSDMA0_CNTL + sdma_offsets[i], f32_cntl);
+	}
+}
+
+/**
  * cik_sdma_enable - stop the async dma engines
  *
  * @adev: amdgpu_device pointer
@@ -537,6 +564,8 @@ static int cik_sdma_start(struct amdgpu_device *adev)
 
 	/* halt the engine before programing */
 	cik_sdma_enable(adev, false);
+	/* enable sdma ring preemption */
+	cik_ctx_switch_enable(adev, true);
 
 	/* start the gfx rings and rlc compute queues */
 	r = cik_sdma_gfx_resume(adev);
@@ -984,6 +1013,7 @@ static int cik_sdma_hw_fini(void *handle)  {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+	cik_ctx_switch_enable(adev, false);
 	cik_sdma_enable(adev, false);
 
 	return 0;
--
1.9.1

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

Re: [PATCH 03/12] drm/amdgpu: Enable SDMA context switching for CIK

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> Enable SDMA context switching on CIK (copied from sdma_v3_0.c).
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

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

> ---
>  drivers/gpu/drm/amd/amdgpu/cik_sdma.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> index c216e16..4a9cea0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> @@ -342,6 +342,33 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)
>  }
>
>  /**
> + * cik_ctx_switch_enable - stop the async dma engines context switch
> + *
> + * @adev: amdgpu_device pointer
> + * @enable: enable/disable the DMA MEs context switch.
> + *
> + * Halt or unhalt the async dma engines context switch (VI).
> + */
> +static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
> +{
> +       u32 f32_cntl;
> +       int i;
> +
> +       for (i = 0; i < adev->sdma.num_instances; i++) {
> +               f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
> +               if (enable) {
> +                       f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
> +                                       AUTO_CTXSW_ENABLE, 1);
> +               } else {
> +                       f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
> +                                       AUTO_CTXSW_ENABLE, 0);
> +               }
> +
> +               WREG32(mmSDMA0_CNTL + sdma_offsets[i], f32_cntl);
> +       }
> +}
> +
> +/**
>   * cik_sdma_enable - stop the async dma engines
>   *
>   * @adev: amdgpu_device pointer
> @@ -537,6 +564,8 @@ static int cik_sdma_start(struct amdgpu_device *adev)
>
>         /* halt the engine before programing */
>         cik_sdma_enable(adev, false);
> +       /* enable sdma ring preemption */
> +       cik_ctx_switch_enable(adev, true);
>
>         /* start the gfx rings and rlc compute queues */
>         r = cik_sdma_gfx_resume(adev);
> @@ -984,6 +1013,7 @@ static int cik_sdma_hw_fini(void *handle)
>  {
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> +       cik_ctx_switch_enable(adev, false);
>         cik_sdma_enable(adev, false);
>
>         return 0;
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 04/12] drm/amdgpu: Make SDMA phase quantum configurable

[Felix Kuehling]

Set a configurable SDMA phase quantum when enabling SDMA context
switching. The default value significantly reduces SDMA latency
in page table updates when user-mode SDMA queues have concurrent
activity, compared to the initial HW setting.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  4 ++++
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c   | 32 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c  | 32 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  | 34 ++++++++++++++++++++++++++++++++-
 5 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 810796a..2129fbb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -106,6 +106,7 @@
 extern unsigned amdgpu_pcie_lane_cap;
 extern unsigned amdgpu_cg_mask;
 extern unsigned amdgpu_pg_mask;
+extern unsigned amdgpu_sdma_phase_quantum;
 extern char *amdgpu_disable_cu;
 extern char *amdgpu_virtual_display;
 extern unsigned amdgpu_pp_feature_mask;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 4bf4a80..02cf24e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -107,6 +107,7 @@
 unsigned amdgpu_pcie_lane_cap = 0;
 unsigned amdgpu_cg_mask = 0xffffffff;
 unsigned amdgpu_pg_mask = 0xffffffff;
+unsigned amdgpu_sdma_phase_quantum = 32;
 char *amdgpu_disable_cu = NULL;
 char *amdgpu_virtual_display = NULL;
 unsigned amdgpu_pp_feature_mask = 0xffffffff;
@@ -223,6 +224,9 @@
 MODULE_PARM_DESC(pg_mask, "Powergating flags mask (0 = disable power gating)");
 module_param_named(pg_mask, amdgpu_pg_mask, uint, 0444);
 
+MODULE_PARM_DESC(sdma_phase_quantum, "SDMA context switch phase quantum (x 1K GPU clock cycles, 0 = no change (default 32))");
+module_param_named(sdma_phase_quantum, amdgpu_sdma_phase_quantum, uint, 0444);
+
 MODULE_PARM_DESC(disable_cu, "Disable CUs (se.sh.cu,...)");
 module_param_named(disable_cu, amdgpu_disable_cu, charp, 0444);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index 4a9cea0..f508f4d 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -351,14 +351,44 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)
  */
 static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
 		if (enable) {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 1);
+			if (amdgpu_sdma_phase_quantum) {
+				WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+				WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+			}
 		} else {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index 67a29fb..b1de44f 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -551,9 +551,33 @@ static void sdma_v3_0_rlc_stop(struct amdgpu_device *adev)
  */
 static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
 		if (enable) {
@@ -561,6 +585,12 @@ static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 					AUTO_CTXSW_ENABLE, 1);
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					ATC_L1_ENABLE, 1);
+			if (amdgpu_sdma_phase_quantum) {
+				WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+				WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+			}
 		} else {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 4a65697..591f3e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -493,13 +493,45 @@ static void sdma_v4_0_rlc_stop(struct amdgpu_device *adev)
  */
 static void sdma_v4_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL));
 		f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 				AUTO_CTXSW_ENABLE, enable ? 1 : 0);
+		if (enable && amdgpu_sdma_phase_quantum) {
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE0_QUANTUM),
+			       phase_quantum);
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE1_QUANTUM),
+			       phase_quantum);
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE2_QUANTUM),
+			       phase_quantum);
+		}
 		WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL), f32_cntl);
 	}
 
-- 
1.9.1

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

RE: [PATCH 04/12] drm/amdgpu: Make SDMA phase quantum configurable

[Zhou, David(ChunMing)]

Acked-by: Chunming Zhou <david1.zhou@amd.com>

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Felix Kuehling
Sent: Tuesday, July 04, 2017 5:11 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix <Felix.Kuehling@amd.com>
Subject: [PATCH 04/12] drm/amdgpu: Make SDMA phase quantum configurable

Set a configurable SDMA phase quantum when enabling SDMA context switching. The default value significantly reduces SDMA latency in page table updates when user-mode SDMA queues have concurrent activity, compared to the initial HW setting.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  4 ++++
 drivers/gpu/drm/amd/amdgpu/cik_sdma.c   | 32 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c  | 32 ++++++++++++++++++++++++++++++-  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  | 34 ++++++++++++++++++++++++++++++++-
 5 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 810796a..2129fbb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -106,6 +106,7 @@
 extern unsigned amdgpu_pcie_lane_cap;
 extern unsigned amdgpu_cg_mask;
 extern unsigned amdgpu_pg_mask;
+extern unsigned amdgpu_sdma_phase_quantum;
 extern char *amdgpu_disable_cu;
 extern char *amdgpu_virtual_display;
 extern unsigned amdgpu_pp_feature_mask; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 4bf4a80..02cf24e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -107,6 +107,7 @@
 unsigned amdgpu_pcie_lane_cap = 0;
 unsigned amdgpu_cg_mask = 0xffffffff;
 unsigned amdgpu_pg_mask = 0xffffffff;
+unsigned amdgpu_sdma_phase_quantum = 32;
 char *amdgpu_disable_cu = NULL;
 char *amdgpu_virtual_display = NULL;
 unsigned amdgpu_pp_feature_mask = 0xffffffff; @@ -223,6 +224,9 @@  MODULE_PARM_DESC(pg_mask, "Powergating flags mask (0 = disable power gating)");  module_param_named(pg_mask, amdgpu_pg_mask, uint, 0444);
 
+MODULE_PARM_DESC(sdma_phase_quantum, "SDMA context switch phase quantum 
+(x 1K GPU clock cycles, 0 = no change (default 32))"); 
+module_param_named(sdma_phase_quantum, amdgpu_sdma_phase_quantum, uint, 
+0444);
+
 MODULE_PARM_DESC(disable_cu, "Disable CUs (se.sh.cu,...)");  module_param_named(disable_cu, amdgpu_disable_cu, charp, 0444);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
index 4a9cea0..f508f4d 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
@@ -351,14 +351,44 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)
  */
 static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool enable)  {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
 		if (enable) {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 1);
+			if (amdgpu_sdma_phase_quantum) {
+				WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+				WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+			}
 		} else {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index 67a29fb..b1de44f 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -551,9 +551,33 @@ static void sdma_v3_0_rlc_stop(struct amdgpu_device *adev)
  */
 static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)  {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
 		if (enable) {
@@ -561,6 +585,12 @@ static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
 					AUTO_CTXSW_ENABLE, 1);
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					ATC_L1_ENABLE, 1);
+			if (amdgpu_sdma_phase_quantum) {
+				WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+				WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
+				       phase_quantum);
+			}
 		} else {
 			f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 					AUTO_CTXSW_ENABLE, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 4a65697..591f3e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -493,13 +493,45 @@ static void sdma_v4_0_rlc_stop(struct amdgpu_device *adev)
  */
 static void sdma_v4_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)  {
-	u32 f32_cntl;
+	u32 f32_cntl, phase_quantum = 0;
 	int i;
 
+	if (amdgpu_sdma_phase_quantum) {
+		unsigned value = amdgpu_sdma_phase_quantum;
+		unsigned unit = 0;
+
+		while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
+			value = (value + 1) >> 1;
+			unit++;
+		}
+		if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+			    SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
+			value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
+				 SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
+			unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
+				SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
+			WARN_ONCE(1,
+			"clamping sdma_phase_quantum to %uK clock cycles\n",
+				  value << unit);
+		}
+		phase_quantum =
+			value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
+			unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
+	}
+
 	for (i = 0; i < adev->sdma.num_instances; i++) {
 		f32_cntl = RREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL));
 		f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
 				AUTO_CTXSW_ENABLE, enable ? 1 : 0);
+		if (enable && amdgpu_sdma_phase_quantum) {
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE0_QUANTUM),
+			       phase_quantum);
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE1_QUANTUM),
+			       phase_quantum);
+			WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE2_QUANTUM),
+			       phase_quantum);
+		}
 		WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL), f32_cntl);
 	}
 
--
1.9.1

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

Re: [PATCH 04/12] drm/amdgpu: Make SDMA phase quantum configurable

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> Set a configurable SDMA phase quantum when enabling SDMA context
> switching. The default value significantly reduces SDMA latency
> in page table updates when user-mode SDMA queues have concurrent
> activity, compared to the initial HW setting.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

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

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  4 ++++
>  drivers/gpu/drm/amd/amdgpu/cik_sdma.c   | 32 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c  | 32 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  | 34 ++++++++++++++++++++++++++++++++-
>  5 files changed, 100 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 810796a..2129fbb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -106,6 +106,7 @@
>  extern unsigned amdgpu_pcie_lane_cap;
>  extern unsigned amdgpu_cg_mask;
>  extern unsigned amdgpu_pg_mask;
> +extern unsigned amdgpu_sdma_phase_quantum;
>  extern char *amdgpu_disable_cu;
>  extern char *amdgpu_virtual_display;
>  extern unsigned amdgpu_pp_feature_mask;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4bf4a80..02cf24e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -107,6 +107,7 @@
>  unsigned amdgpu_pcie_lane_cap = 0;
>  unsigned amdgpu_cg_mask = 0xffffffff;
>  unsigned amdgpu_pg_mask = 0xffffffff;
> +unsigned amdgpu_sdma_phase_quantum = 32;
>  char *amdgpu_disable_cu = NULL;
>  char *amdgpu_virtual_display = NULL;
>  unsigned amdgpu_pp_feature_mask = 0xffffffff;
> @@ -223,6 +224,9 @@
>  MODULE_PARM_DESC(pg_mask, "Powergating flags mask (0 = disable power gating)");
>  module_param_named(pg_mask, amdgpu_pg_mask, uint, 0444);
>
> +MODULE_PARM_DESC(sdma_phase_quantum, "SDMA context switch phase quantum (x 1K GPU clock cycles, 0 = no change (default 32))");
> +module_param_named(sdma_phase_quantum, amdgpu_sdma_phase_quantum, uint, 0444);
> +
>  MODULE_PARM_DESC(disable_cu, "Disable CUs (se.sh.cu,...)");
>  module_param_named(disable_cu, amdgpu_disable_cu, charp, 0444);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> index 4a9cea0..f508f4d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
> @@ -351,14 +351,44 @@ static void cik_sdma_rlc_stop(struct amdgpu_device *adev)
>   */
>  static void cik_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
>  {
> -       u32 f32_cntl;
> +       u32 f32_cntl, phase_quantum = 0;
>         int i;
>
> +       if (amdgpu_sdma_phase_quantum) {
> +               unsigned value = amdgpu_sdma_phase_quantum;
> +               unsigned unit = 0;
> +
> +               while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
> +                       value = (value + 1) >> 1;
> +                       unit++;
> +               }
> +               if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                           SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
> +                       value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                                SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
> +                       unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
> +                       WARN_ONCE(1,
> +                       "clamping sdma_phase_quantum to %uK clock cycles\n",
> +                                 value << unit);
> +               }
> +               phase_quantum =
> +                       value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
> +                       unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
> +       }
> +
>         for (i = 0; i < adev->sdma.num_instances; i++) {
>                 f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
>                 if (enable) {
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         AUTO_CTXSW_ENABLE, 1);
> +                       if (amdgpu_sdma_phase_quantum) {
> +                               WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
> +                                      phase_quantum);
> +                               WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
> +                                      phase_quantum);
> +                       }
>                 } else {
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         AUTO_CTXSW_ENABLE, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> index 67a29fb..b1de44f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
> @@ -551,9 +551,33 @@ static void sdma_v3_0_rlc_stop(struct amdgpu_device *adev)
>   */
>  static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
>  {
> -       u32 f32_cntl;
> +       u32 f32_cntl, phase_quantum = 0;
>         int i;
>
> +       if (amdgpu_sdma_phase_quantum) {
> +               unsigned value = amdgpu_sdma_phase_quantum;
> +               unsigned unit = 0;
> +
> +               while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
> +                       value = (value + 1) >> 1;
> +                       unit++;
> +               }
> +               if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                           SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
> +                       value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                                SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
> +                       unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
> +                       WARN_ONCE(1,
> +                       "clamping sdma_phase_quantum to %uK clock cycles\n",
> +                                 value << unit);
> +               }
> +               phase_quantum =
> +                       value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
> +                       unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
> +       }
> +
>         for (i = 0; i < adev->sdma.num_instances; i++) {
>                 f32_cntl = RREG32(mmSDMA0_CNTL + sdma_offsets[i]);
>                 if (enable) {
> @@ -561,6 +585,12 @@ static void sdma_v3_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
>                                         AUTO_CTXSW_ENABLE, 1);
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         ATC_L1_ENABLE, 1);
> +                       if (amdgpu_sdma_phase_quantum) {
> +                               WREG32(mmSDMA0_PHASE0_QUANTUM + sdma_offsets[i],
> +                                      phase_quantum);
> +                               WREG32(mmSDMA0_PHASE1_QUANTUM + sdma_offsets[i],
> +                                      phase_quantum);
> +                       }
>                 } else {
>                         f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                         AUTO_CTXSW_ENABLE, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 4a65697..591f3e7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -493,13 +493,45 @@ static void sdma_v4_0_rlc_stop(struct amdgpu_device *adev)
>   */
>  static void sdma_v4_0_ctx_switch_enable(struct amdgpu_device *adev, bool enable)
>  {
> -       u32 f32_cntl;
> +       u32 f32_cntl, phase_quantum = 0;
>         int i;
>
> +       if (amdgpu_sdma_phase_quantum) {
> +               unsigned value = amdgpu_sdma_phase_quantum;
> +               unsigned unit = 0;
> +
> +               while (value > (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__VALUE__SHIFT)) {
> +                       value = (value + 1) >> 1;
> +                       unit++;
> +               }
> +               if (unit > (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                           SDMA0_PHASE0_QUANTUM__UNIT__SHIFT)) {
> +                       value = (SDMA0_PHASE0_QUANTUM__VALUE_MASK >>
> +                                SDMA0_PHASE0_QUANTUM__VALUE__SHIFT);
> +                       unit = (SDMA0_PHASE0_QUANTUM__UNIT_MASK >>
> +                               SDMA0_PHASE0_QUANTUM__UNIT__SHIFT);
> +                       WARN_ONCE(1,
> +                       "clamping sdma_phase_quantum to %uK clock cycles\n",
> +                                 value << unit);
> +               }
> +               phase_quantum =
> +                       value << SDMA0_PHASE0_QUANTUM__VALUE__SHIFT |
> +                       unit  << SDMA0_PHASE0_QUANTUM__UNIT__SHIFT;
> +       }
> +
>         for (i = 0; i < adev->sdma.num_instances; i++) {
>                 f32_cntl = RREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL));
>                 f32_cntl = REG_SET_FIELD(f32_cntl, SDMA0_CNTL,
>                                 AUTO_CTXSW_ENABLE, enable ? 1 : 0);
> +               if (enable && amdgpu_sdma_phase_quantum) {
> +                       WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE0_QUANTUM),
> +                              phase_quantum);
> +                       WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE1_QUANTUM),
> +                              phase_quantum);
> +                       WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_PHASE2_QUANTUM),
> +                              phase_quantum);
> +               }
>                 WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_CNTL), f32_cntl);
>         }
>
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Felix Kuehling]

From: Jay Cornwall <Jay.Cornwall@amd.com>

A subset of VM fault types currently send retry XNACK to the client.
This causes a storm of interrupts from the VM to the host.

Until the storm is throttled by other means send no-retry XNACK for
all fault types instead. No change in behavior to the client which
will stall indefinitely with the current configuration in any case.
Improves system stability under GC or MMHUB faults.

Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index a42f483..f957b18 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
 		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
 				PAGE_TABLE_BLOCK_SIZE,
 				adev->vm_manager.block_size - 9);
+		/* Send no-retry XNACK on fault to suppress VM fault storm. */
+		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
+				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
 		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
 		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
 		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index 01918dc..b760018 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
 		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
 				PAGE_TABLE_BLOCK_SIZE,
 				adev->vm_manager.block_size - 9);
+		/* Send no-retry XNACK on fault to suppress VM fault storm. */
+		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
+				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
 		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i, tmp);
 		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
 		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
-- 
1.9.1

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

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Felix Kuehling]

Any comments?

I believe this is a nice stability improvement. In case of VM faults
they don't take down the whole GPU with an interrupt storm. With KFD we
can recover without a GPU reset in many cases just by unmapping the
offending process' queues.

Regards,
  Felix


On 17-07-03 05:11 PM, Felix Kuehling wrote:
> From: Jay Cornwall <Jay.Cornwall@amd.com>
>
> A subset of VM fault types currently send retry XNACK to the client.
> This causes a storm of interrupts from the VM to the host.
>
> Until the storm is throttled by other means send no-retry XNACK for
> all fault types instead. No change in behavior to the client which
> will stall indefinitely with the current configuration in any case.
> Improves system stability under GC or MMHUB faults.
>
> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> index a42f483..f957b18 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>  		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>  				PAGE_TABLE_BLOCK_SIZE,
>  				adev->vm_manager.block_size - 9);
> +		/* Send no-retry XNACK on fault to suppress VM fault storm. */
> +		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
> +				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>  		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>  		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>  		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> index 01918dc..b760018 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>  		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>  				PAGE_TABLE_BLOCK_SIZE,
>  				adev->vm_manager.block_size - 9);
> +		/* Send no-retry XNACK on fault to suppress VM fault storm. */
> +		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
> +				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>  		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>  		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>  		WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);

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

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Alex Deucher]

On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> Any comments?
>
> I believe this is a nice stability improvement. In case of VM faults
> they don't take down the whole GPU with an interrupt storm. With KFD we
> can recover without a GPU reset in many cases just by unmapping the
> offending process' queues.

Will this cause any problems with enabling recoverable page faults
later?  If not,
Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
> Regards,
>   Felix
>
>
> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>
>> A subset of VM fault types currently send retry XNACK to the client.
>> This causes a storm of interrupts from the VM to the host.
>>
>> Until the storm is throttled by other means send no-retry XNACK for
>> all fault types instead. No change in behavior to the client which
>> will stall indefinitely with the current configuration in any case.
>> Improves system stability under GC or MMHUB faults.
>>
>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> index a42f483..f957b18 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>                               PAGE_TABLE_BLOCK_SIZE,
>>                               adev->vm_manager.block_size - 9);
>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> index 01918dc..b760018 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>                               PAGE_TABLE_BLOCK_SIZE,
>>                               adev->vm_manager.block_size - 9);
>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Bridgman, John]

>-----Original Message-----
>From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>Of Alex Deucher
>Sent: Wednesday, July 12, 2017 11:59 AM
>To: Kuehling, Felix
>Cc: amd-gfx list
>Subject: Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault
>types
>
>On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com>
>wrote:
>> Any comments?
>>
>> I believe this is a nice stability improvement. In case of VM faults
>> they don't take down the whole GPU with an interrupt storm. With KFD
>> we can recover without a GPU reset in many cases just by unmapping the
>> offending process' queues.
>
>Will this cause any problems with enabling recoverable page faults later?  If
>not,
>Acked-by: Alex Deucher <alexander.deucher@amd.com>

We will need to back this out in order to enable recoverable page faults later, but probably still worth doing in the short term IMO.

>
>>
>> Regards,
>>   Felix
>>
>>
>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>>
>>> A subset of VM fault types currently send retry XNACK to the client.
>>> This causes a storm of interrupts from the VM to the host.
>>>
>>> Until the storm is throttled by other means send no-retry XNACK for
>>> all fault types instead. No change in behavior to the client which
>>> will stall indefinitely with the current configuration in any case.
>>> Improves system stability under GC or MMHUB faults.
>>>
>>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>>> drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> index a42f483..f957b18 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct
>amdgpu_device *adev)
>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>                               adev->vm_manager.block_size - 9);
>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>> +
>>> + RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>               WREG32_SOC15_OFFSET(GC, 0,
>mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>               WREG32_SOC15_OFFSET(GC, 0,
>>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> index 01918dc..b760018 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct
>amdgpu_device *adev)
>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>                               adev->vm_manager.block_size - 9);
>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>> +
>>> + RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i,
>tmp);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0,
>mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0,
>>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>_______________________________________________
>amd-gfx mailing list
>amd-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Christian König]

Am 12.07.2017 um 18:15 schrieb Bridgman, John:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Alex Deucher
>> Sent: Wednesday, July 12, 2017 11:59 AM
>> To: Kuehling, Felix
>> Cc: amd-gfx list
>> Subject: Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault
>> types
>>
>> On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com>
>> wrote:
>>> Any comments?
>>>
>>> I believe this is a nice stability improvement. In case of VM faults
>>> they don't take down the whole GPU with an interrupt storm. With KFD
>>> we can recover without a GPU reset in many cases just by unmapping the
>>> offending process' queues.
>> Will this cause any problems with enabling recoverable page faults later?  If
>> not,
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> We will need to back this out in order to enable recoverable page faults later, but probably still worth doing in the short term IMO.

Yeah, agree. Especially avoiding the interrupt ring overflow sounds like 
a good idea to me.

Patch is Acked-by: Christian König <christian.koenig@amd.com> as well.

Christian.

>
>>> Regards,
>>>    Felix
>>>
>>>
>>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>>>
>>>> A subset of VM fault types currently send retry XNACK to the client.
>>>> This causes a storm of interrupts from the VM to the host.
>>>>
>>>> Until the storm is throttled by other means send no-retry XNACK for
>>>> all fault types instead. No change in behavior to the client which
>>>> will stall indefinitely with the current configuration in any case.
>>>> Improves system stability under GC or MMHUB faults.
>>>>
>>>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>>>> drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>>>   2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> index a42f483..f957b18 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct
>> amdgpu_device *adev)
>>>>                tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>>                                PAGE_TABLE_BLOCK_SIZE,
>>>>                                adev->vm_manager.block_size - 9);
>>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>> +
>>>> + RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>>                WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>>                WREG32_SOC15_OFFSET(GC, 0,
>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>>                WREG32_SOC15_OFFSET(GC, 0,
>>>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0); diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> index 01918dc..b760018 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct
>> amdgpu_device *adev)
>>>>                tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>>                                PAGE_TABLE_BLOCK_SIZE,
>>>>                                adev->vm_manager.block_size - 9);
>>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>> +
>>>> + RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>>                WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i,
>> tmp);
>>>>                WREG32_SOC15_OFFSET(MMHUB, 0,
>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>>                WREG32_SOC15_OFFSET(MMHUB, 0,
>>>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Felix Kuehling]

On 17-07-12 11:59 AM, Alex Deucher wrote:
> On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com> wrote:
>> Any comments?
>>
>> I believe this is a nice stability improvement. In case of VM faults
>> they don't take down the whole GPU with an interrupt storm. With KFD we
>> can recover without a GPU reset in many cases just by unmapping the
>> offending process' queues.
> Will this cause any problems with enabling recoverable page faults
> later?  If not,
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

Like John said, this will need to be backed out when we enable
recoverable page faults. The nice thing on Vega10 is, that it's a
per-VMID setting. That will allow us for example to enable recoverable
page faults for KFD VMIDs for implementing a real HSA memory model,
without affecting the graphics VMIDs.

Still OK to add your Acked-by?

Regards,
  Felix

>
>> Regards,
>>   Felix
>>
>>
>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>>
>>> A subset of VM fault types currently send retry XNACK to the client.
>>> This causes a storm of interrupts from the VM to the host.
>>>
>>> Until the storm is throttled by other means send no-retry XNACK for
>>> all fault types instead. No change in behavior to the client which
>>> will stall indefinitely with the current configuration in any case.
>>> Improves system stability under GC or MMHUB faults.
>>>
>>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>>>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> index a42f483..f957b18 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>                               adev->vm_manager.block_size - 9);
>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> index 01918dc..b760018 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>                               adev->vm_manager.block_size - 9);
>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Alex Deucher]

On Wed, Jul 12, 2017 at 1:37 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> On 17-07-12 11:59 AM, Alex Deucher wrote:
>> On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com> wrote:
>>> Any comments?
>>>
>>> I believe this is a nice stability improvement. In case of VM faults
>>> they don't take down the whole GPU with an interrupt storm. With KFD we
>>> can recover without a GPU reset in many cases just by unmapping the
>>> offending process' queues.
>> Will this cause any problems with enabling recoverable page faults
>> later?  If not,
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>
> Like John said, this will need to be backed out when we enable
> recoverable page faults. The nice thing on Vega10 is, that it's a
> per-VMID setting. That will allow us for example to enable recoverable
> page faults for KFD VMIDs for implementing a real HSA memory model,
> without affecting the graphics VMIDs.
>
> Still OK to add your Acked-by?

Yes, go ahead.

Alex

>
> Regards,
>   Felix
>
>>
>>> Regards,
>>>   Felix
>>>
>>>
>>> On 17-07-03 05:11 PM, Felix Kuehling wrote:
>>>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>>>
>>>> A subset of VM fault types currently send retry XNACK to the client.
>>>> This causes a storm of interrupts from the VM to the host.
>>>>
>>>> Until the storm is throttled by other means send no-retry XNACK for
>>>> all fault types instead. No change in behavior to the client which
>>>> will stall indefinitely with the current configuration in any case.
>>>> Improves system stability under GC or MMHUB faults.
>>>>
>>>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>>>>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> index a42f483..f957b18 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>>>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>>                               adev->vm_manager.block_size - 9);
>>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>>               WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> index 01918dc..b760018 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>>>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct amdgpu_device *adev)
>>>>               tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>>                               PAGE_TABLE_BLOCK_SIZE,
>>>>                               adev->vm_manager.block_size - 9);
>>>> +             /* Send no-retry XNACK on fault to suppress VM fault storm. */
>>>> +             tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>>> +                                 RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_CNTL, i, tmp);
>>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>>>               WREG32_SOC15_OFFSET(MMHUB, 0, mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Jay Cornwall]

On Wed, Jul 12, 2017, at 12:37, Felix Kuehling wrote:
> On 17-07-12 11:59 AM, Alex Deucher wrote:
> > On Wed, Jul 12, 2017 at 1:40 AM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> >> Any comments?
> >>
> >> I believe this is a nice stability improvement. In case of VM faults
> >> they don't take down the whole GPU with an interrupt storm. With KFD we
> >> can recover without a GPU reset in many cases just by unmapping the
> >> offending process' queues.
> > Will this cause any problems with enabling recoverable page faults
> > later?  If not,
> > Acked-by: Alex Deucher <alexander.deucher@amd.com>
> 
> Like John said, this will need to be backed out when we enable
> recoverable page faults. The nice thing on Vega10 is, that it's a
> per-VMID setting. That will allow us for example to enable recoverable
> page faults for KFD VMIDs for implementing a real HSA memory model,
> without affecting the graphics VMIDs.

Right, the plan is to re-enable this feature once the interrupt storm
has been resolved. There are a few options for this discussed internally
but not currently implemented as far as I know.

I have a backup plan for implementing recoverable page faults with
no-retry XNACK if that doesn't pan out.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault types

[Bridgman, John]

Agreed... I thought we had already made this change but if not then... 

Reviewed-by: John Bridgman <John.Bridgman@amd.com>

>-----Original Message-----
>From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>Of Felix Kuehling
>Sent: Wednesday, July 12, 2017 1:41 AM
>To: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH 05/12] drm/amdgpu: Send no-retry XNACK for all fault
>types
>
>Any comments?
>
>I believe this is a nice stability improvement. In case of VM faults they don't
>take down the whole GPU with an interrupt storm. With KFD we can recover
>without a GPU reset in many cases just by unmapping the offending process'
>queues.
>
>Regards,
>  Felix
>
>
>On 17-07-03 05:11 PM, Felix Kuehling wrote:
>> From: Jay Cornwall <Jay.Cornwall@amd.com>
>>
>> A subset of VM fault types currently send retry XNACK to the client.
>> This causes a storm of interrupts from the VM to the host.
>>
>> Until the storm is throttled by other means send no-retry XNACK for
>> all fault types instead. No change in behavior to the client which
>> will stall indefinitely with the current configuration in any case.
>> Improves system stability under GC or MMHUB faults.
>>
>> Signed-off-by: Jay Cornwall <Jay.Cornwall@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 3 +++
>> drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> index a42f483..f957b18 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
>> @@ -206,6 +206,9 @@ static void gfxhub_v1_0_setup_vmid_config(struct
>amdgpu_device *adev)
>>  		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>  				PAGE_TABLE_BLOCK_SIZE,
>>  				adev->vm_manager.block_size - 9);
>> +		/* Send no-retry XNACK on fault to suppress VM fault storm.
>*/
>> +		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>> +
>RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>  		WREG32_SOC15_OFFSET(GC, 0, mmVM_CONTEXT1_CNTL, i,
>tmp);
>>  		WREG32_SOC15_OFFSET(GC, 0,
>mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>  		WREG32_SOC15_OFFSET(GC, 0,
>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0); diff --git
>> a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> index 01918dc..b760018 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
>> @@ -222,6 +222,9 @@ static void mmhub_v1_0_setup_vmid_config(struct
>amdgpu_device *adev)
>>  		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>>  				PAGE_TABLE_BLOCK_SIZE,
>>  				adev->vm_manager.block_size - 9);
>> +		/* Send no-retry XNACK on fault to suppress VM fault storm.
>*/
>> +		tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
>> +
>RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 0);
>>  		WREG32_SOC15_OFFSET(MMHUB, 0,
>mmVM_CONTEXT1_CNTL, i, tmp);
>>  		WREG32_SOC15_OFFSET(MMHUB, 0,
>mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32, i*2, 0);
>>  		WREG32_SOC15_OFFSET(MMHUB, 0,
>> mmVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32, i*2, 0);
>
>_______________________________________________
>amd-gfx mailing list
>amd-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Felix Kuehling]

From: Yong Zhao <yong.zhao@amd.com>

Signed-off-by: Yong Zhao <yong.zhao@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
 drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5b1220f..bc69b9c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -56,6 +56,8 @@
 #include <linux/firmware.h>
 #include "amdgpu_vf_error.h"
 
+#include "amdgpu_amdkfd.h"
+
 MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
 MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
 
@@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
 		drm_modeset_unlock_all(dev);
 	}
 
+	amdgpu_amdkfd_suspend(adev);
+
 	/* unpin the front buffers and cursors */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
@@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
 			}
 		}
 	}
+	r = amdgpu_amdkfd_resume(adev);
+	if (r)
+		return r;
 
 	/* blat the mode back in */
 	if (fbcon) {
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index 6ce9f80..00639bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	amdgpu_amdkfd_suspend(adev);
-
 	return cik_common_hw_fini(adev);
 }
 
 static int cik_common_resume(void *handle)
 {
-	int r;
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	r = cik_common_hw_init(adev);
-	if (r)
-		return r;
-
-	return amdgpu_amdkfd_resume(adev);
+	return cik_common_hw_init(adev);
 }
 
 static bool cik_common_is_idle(void *handle)
-- 
1.9.1

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

Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Oded Gabbay]

On Tue, Jul 4, 2017 at 12:11 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Yong Zhao <yong.zhao@amd.com>
>
> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
>  drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b1220f..bc69b9c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -56,6 +56,8 @@
>  #include <linux/firmware.h>
>  #include "amdgpu_vf_error.h"
>
> +#include "amdgpu_amdkfd.h"
> +
>  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>
> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>                 drm_modeset_unlock_all(dev);
>         }
>
> +       amdgpu_amdkfd_suspend(adev);
> +
>         /* unpin the front buffers and cursors */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>                 struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>                         }
>                 }
>         }
> +       r = amdgpu_amdkfd_resume(adev);
> +       if (r)
> +               return r;
>
>         /* blat the mode back in */
>         if (fbcon) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 6ce9f80..00639bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>  {
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -       amdgpu_amdkfd_suspend(adev);
> -
>         return cik_common_hw_fini(adev);
>  }
>
>  static int cik_common_resume(void *handle)
>  {
> -       int r;
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -       r = cik_common_hw_init(adev);
> -       if (r)
> -               return r;
> -
> -       return amdgpu_amdkfd_resume(adev);
> +       return cik_common_hw_init(adev);
>  }
>
>  static bool cik_common_is_idle(void *handle)
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Yong Zhao <yong.zhao@amd.com>
>
> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Does this work properly for multiple GPUs?  E.g., if one is suspended
and another is not?  E.g., PX laptops where we runtime suspend the
dGPU while the APU is still running.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
>  drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b1220f..bc69b9c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -56,6 +56,8 @@
>  #include <linux/firmware.h>
>  #include "amdgpu_vf_error.h"
>
> +#include "amdgpu_amdkfd.h"
> +
>  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>
> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>                 drm_modeset_unlock_all(dev);
>         }
>
> +       amdgpu_amdkfd_suspend(adev);
> +
>         /* unpin the front buffers and cursors */
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>                 struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>                         }
>                 }
>         }
> +       r = amdgpu_amdkfd_resume(adev);
> +       if (r)
> +               return r;
>
>         /* blat the mode back in */
>         if (fbcon) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 6ce9f80..00639bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>  {
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -       amdgpu_amdkfd_suspend(adev);
> -
>         return cik_common_hw_fini(adev);
>  }
>
>  static int cik_common_resume(void *handle)
>  {
> -       int r;
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -       r = cik_common_hw_init(adev);
> -       if (r)
> -               return r;
> -
> -       return amdgpu_amdkfd_resume(adev);
> +       return cik_common_hw_init(adev);
>  }
>
>  static bool cik_common_is_idle(void *handle)
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Yong Zhao]

Hi Alex,

As far as I know, we never tested suspend/resume on the setting you 
mentioned. Theoretically it should work.

When I read the code now, I was wondering whether we should stop kfd 
before amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If 
that's not needed, it may make more sense to stick to the previous 
design which kept the kfd suspend/resume inside your IP block 
suspend/resume.

Regards,

Yong


On 2017-07-06 05:06 PM, Alex Deucher wrote:
> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> From: Yong Zhao <yong.zhao@amd.com>
>>
>> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Does this work properly for multiple GPUs?  E.g., if one is suspended
> and another is not?  E.g., PX laptops where we runtime suspend the
> dGPU while the APU is still running.
>
> Alex
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
>>   drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 5b1220f..bc69b9c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -56,6 +56,8 @@
>>   #include <linux/firmware.h>
>>   #include "amdgpu_vf_error.h"
>>
>> +#include "amdgpu_amdkfd.h"
>> +
>>   MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>>   MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>>
>> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>>                  drm_modeset_unlock_all(dev);
>>          }
>>
>> +       amdgpu_amdkfd_suspend(adev);
>> +
>>          /* unpin the front buffers and cursors */
>>          list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>                  struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
>>                          }
>>                  }
>>          }
>> +       r = amdgpu_amdkfd_resume(adev);
>> +       if (r)
>> +               return r;
>>
>>          /* blat the mode back in */
>>          if (fbcon) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
>> index 6ce9f80..00639bf 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>>   {
>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>
>> -       amdgpu_amdkfd_suspend(adev);
>> -
>>          return cik_common_hw_fini(adev);
>>   }
>>
>>   static int cik_common_resume(void *handle)
>>   {
>> -       int r;
>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>
>> -       r = cik_common_hw_init(adev);
>> -       if (r)
>> -               return r;
>> -
>> -       return amdgpu_amdkfd_resume(adev);
>> +       return cik_common_hw_init(adev);
>>   }
>>
>>   static bool cik_common_is_idle(void *handle)
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Alex Deucher]

On Thu, Jul 6, 2017 at 5:33 PM, Yong Zhao <yong.zhao@amd.com> wrote:
> Hi Alex,
>
> As far as I know, we never tested suspend/resume on the setting you
> mentioned. Theoretically it should work.

Are the kfd s/r entry points global or per GPU?  If you have two GPUs
and you suspend one, will it suspend the entire kfd?  I'm fine with
the change, it's no worse than the current situation.  Mostly just
curious.

>
> When I read the code now, I was wondering whether we should stop kfd before
> amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If that's not
> needed, it may make more sense to stick to the previous design which kept
> the kfd suspend/resume inside your IP block suspend/resume.

I think it makes more sense to put the kfd calls in the common device
s/r code rather than in the soc specific ip functions.  Change is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


>
> Regards,
>
> Yong
>
>
>
> On 2017-07-06 05:06 PM, Alex Deucher wrote:
>>
>> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com>
>> wrote:
>>>
>>> From: Yong Zhao <yong.zhao@amd.com>
>>>
>>> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>
>> Does this work properly for multiple GPUs?  E.g., if one is suspended
>> and another is not?  E.g., PX laptops where we runtime suspend the
>> dGPU while the APU is still running.
>>
>> Alex
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
>>>   drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 5b1220f..bc69b9c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -56,6 +56,8 @@
>>>   #include <linux/firmware.h>
>>>   #include "amdgpu_vf_error.h"
>>>
>>> +#include "amdgpu_amdkfd.h"
>>> +
>>>   MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>>>   MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>>>
>>> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev,
>>> bool suspend, bool fbcon)
>>>                  drm_modeset_unlock_all(dev);
>>>          }
>>>
>>> +       amdgpu_amdkfd_suspend(adev);
>>> +
>>>          /* unpin the front buffers and cursors */
>>>          list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>>                  struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>>> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev,
>>> bool resume, bool fbcon)
>>>                          }
>>>                  }
>>>          }
>>> +       r = amdgpu_amdkfd_resume(adev);
>>> +       if (r)
>>> +               return r;
>>>
>>>          /* blat the mode back in */
>>>          if (fbcon) {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 6ce9f80..00639bf 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>>>   {
>>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>
>>> -       amdgpu_amdkfd_suspend(adev);
>>> -
>>>          return cik_common_hw_fini(adev);
>>>   }
>>>
>>>   static int cik_common_resume(void *handle)
>>>   {
>>> -       int r;
>>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>
>>> -       r = cik_common_hw_init(adev);
>>> -       if (r)
>>> -               return r;
>>> -
>>> -       return amdgpu_amdkfd_resume(adev);
>>> +       return cik_common_hw_init(adev);
>>>   }
>>>
>>>   static bool cik_common_is_idle(void *handle)
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

[Yong Zhao]

On 2017-07-06 05:44 PM, Alex Deucher wrote:
> On Thu, Jul 6, 2017 at 5:33 PM, Yong Zhao <yong.zhao@amd.com> wrote:
>> Hi Alex,
>>
>> As far as I know, we never tested suspend/resume on the setting you
>> mentioned. Theoretically it should work.
> Are the kfd s/r entry points global or per GPU?  If you have two GPUs
> and you suspend one, will it suspend the entire kfd?  I'm fine with
> the change, it's no worse than the current situation.  Mostly just
> curious.
kfd s/r is per GPU. If we suspend only one out of two GPUs, the other 
one will keep working.
>
>> When I read the code now, I was wondering whether we should stop kfd before
>> amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If that's not
>> needed, it may make more sense to stick to the previous design which kept
>> the kfd suspend/resume inside your IP block suspend/resume.
> I think it makes more sense to put the kfd calls in the common device
> s/r code rather than in the soc specific ip functions.  Change is:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
>
>> Regards,
>>
>> Yong
>>
>>
>>
>> On 2017-07-06 05:06 PM, Alex Deucher wrote:
>>> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com>
>>> wrote:
>>>> From: Yong Zhao <yong.zhao@amd.com>
>>>>
>>>> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Does this work properly for multiple GPUs?  E.g., if one is suspended
>>> and another is not?  E.g., PX laptops where we runtime suspend the
>>> dGPU while the APU is still running.
>>>
>>> Alex
>>>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++++
>>>>    drivers/gpu/drm/amd/amdgpu/cik.c           | 9 +--------
>>>>    2 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 5b1220f..bc69b9c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -56,6 +56,8 @@
>>>>    #include <linux/firmware.h>
>>>>    #include "amdgpu_vf_error.h"
>>>>
>>>> +#include "amdgpu_amdkfd.h"
>>>> +
>>>>    MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>>>>    MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>>>>
>>>> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev,
>>>> bool suspend, bool fbcon)
>>>>                   drm_modeset_unlock_all(dev);
>>>>           }
>>>>
>>>> +       amdgpu_amdkfd_suspend(adev);
>>>> +
>>>>           /* unpin the front buffers and cursors */
>>>>           list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>>>                   struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>>>> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev,
>>>> bool resume, bool fbcon)
>>>>                           }
>>>>                   }
>>>>           }
>>>> +       r = amdgpu_amdkfd_resume(adev);
>>>> +       if (r)
>>>> +               return r;
>>>>
>>>>           /* blat the mode back in */
>>>>           if (fbcon) {
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> index 6ce9f80..00639bf 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>>> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>>>>    {
>>>>           struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>>
>>>> -       amdgpu_amdkfd_suspend(adev);
>>>> -
>>>>           return cik_common_hw_fini(adev);
>>>>    }
>>>>
>>>>    static int cik_common_resume(void *handle)
>>>>    {
>>>> -       int r;
>>>>           struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>>
>>>> -       r = cik_common_hw_init(adev);
>>>> -       if (r)
>>>> -               return r;
>>>> -
>>>> -       return amdgpu_amdkfd_resume(adev);
>>>> +       return cik_common_hw_init(adev);
>>>>    }
>>>>
>>>>    static bool cik_common_is_idle(void *handle)
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>

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

[PATCH 07/12] drm: export drm_gem_prime_dmabuf_ops

[Felix Kuehling]

From: Christian König <christian.koenig@amd.com>

This allows drivers to check if a DMA-buf contains a GEM object or not.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/drm_prime.c | 3 ++-
 include/drm/drmP.h          | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 25aa455..5cb4fd9 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -396,7 +396,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
 	return dev->driver->gem_prime_mmap(obj, vma);
 }
 
-static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
+const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
 	.attach = drm_gem_map_attach,
 	.detach = drm_gem_map_detach,
 	.map_dma_buf = drm_gem_map_dma_buf,
@@ -410,6 +410,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
 	.vmap = drm_gem_dmabuf_vmap,
 	.vunmap = drm_gem_dmabuf_vunmap,
 };
+EXPORT_SYMBOL(drm_gem_prime_dmabuf_ops);
 
 /**
  * DOC: PRIME Helpers
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6105c05..e0ea8f8 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -761,6 +761,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
 
 struct dma_buf_export_info;
 
+extern const struct dma_buf_ops drm_gem_prime_dmabuf_ops;
+
 extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 					    struct drm_gem_object *obj,
 					    int flags);
-- 
1.9.1

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

Re: [PATCH 07/12] drm: export drm_gem_prime_dmabuf_ops

[Michel Dänzer]

Adding the dri-devel list, since this is a core DRM patch.


On 04/07/17 06:11 AM, Felix Kuehling wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> This allows drivers to check if a DMA-buf contains a GEM object or not.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 3 ++-
>  include/drm/drmP.h          | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 25aa455..5cb4fd9 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -396,7 +396,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
>  	return dev->driver->gem_prime_mmap(obj, vma);
>  }
>  
> -static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> +const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>  	.attach = drm_gem_map_attach,
>  	.detach = drm_gem_map_detach,
>  	.map_dma_buf = drm_gem_map_dma_buf,
> @@ -410,6 +410,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
>  	.vmap = drm_gem_dmabuf_vmap,
>  	.vunmap = drm_gem_dmabuf_vunmap,
>  };
> +EXPORT_SYMBOL(drm_gem_prime_dmabuf_ops);
>  
>  /**
>   * DOC: PRIME Helpers
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6105c05..e0ea8f8 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -761,6 +761,8 @@ static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
>  
>  struct dma_buf_export_info;
>  
> +extern const struct dma_buf_ops drm_gem_prime_dmabuf_ops;
> +
>  extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>  					    struct drm_gem_object *obj,
>  					    int flags);
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 07/12] drm: export drm_gem_prime_dmabuf_ops

[Dave Airlie]

[-- Attachment #1.1: Type: text/plain, Size: 2472 bytes --]

On 4 Jul. 2017 11:23, "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org> wrote:


Adding the dri-devel list, since this is a core DRM patch.


On 04/07/17 06:11 AM, Felix Kuehling wrote:
> From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>
> This allows drivers to check if a DMA-buf contains a GEM object or not.


Please use an accessor function. I doubt it'll be a fast path.

Dave.

>
> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> Reviewed-by: Felix Kuehling <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/gpu/drm/drm_prime.c | 3 ++-
>  include/drm/drmP.h          | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 25aa455..5cb4fd9 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -396,7 +396,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
*dma_buf,
>       return dev->driver->gem_prime_mmap(obj, vma);
>  }
>
> -static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> +const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>       .attach = drm_gem_map_attach,
>       .detach = drm_gem_map_detach,
>       .map_dma_buf = drm_gem_map_dma_buf,
> @@ -410,6 +410,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
*dma_buf,
>       .vmap = drm_gem_dmabuf_vmap,
>       .vunmap = drm_gem_dmabuf_vunmap,
>  };
> +EXPORT_SYMBOL(drm_gem_prime_dmabuf_ops);
>
>  /**
>   * DOC: PRIME Helpers
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6105c05..e0ea8f8 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -761,6 +761,8 @@ static inline int drm_debugfs_remove_files(const
struct drm_info_list *files,
>
>  struct dma_buf_export_info;
>
> +extern const struct dma_buf_ops drm_gem_prime_dmabuf_ops;
> +
>  extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>                                           struct drm_gem_object *obj,
>                                           int flags);
>


--
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: Type: text/html, Size: 4106 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 07/12] drm: export drm_gem_prime_dmabuf_ops

[Daniel Vetter]

On Tue, Jul 04, 2017 at 02:55:00PM +1000, Dave Airlie wrote:
> On 4 Jul. 2017 11:23, "Michel Dänzer" <michel@daenzer.net> wrote:
> 
> 
> Adding the dri-devel list, since this is a core DRM patch.
> 
> 
> On 04/07/17 06:11 AM, Felix Kuehling wrote:
> > From: Christian König <christian.koenig@amd.com>
> >
> > This allows drivers to check if a DMA-buf contains a GEM object or not.
> 
> 
> Please use an accessor function. I doubt it'll be a fast path.

I think even better if that helper checks against the driver instance too,
since that's really the answer you want drm_prime_try_cast_to_gem_bo or
similar, which will return NULL if it's not a prime dma-buf or not one for
the current driver instance. Feel free to improve the naming :-)
-Daniel

> 
> Dave.
> 
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> > ---
> >  drivers/gpu/drm/drm_prime.c | 3 ++-
> >  include/drm/drmP.h          | 2 ++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 25aa455..5cb4fd9 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -396,7 +396,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
> *dma_buf,
> >       return dev->driver->gem_prime_mmap(obj, vma);
> >  }
> >
> > -static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> > +const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> >       .attach = drm_gem_map_attach,
> >       .detach = drm_gem_map_detach,
> >       .map_dma_buf = drm_gem_map_dma_buf,
> > @@ -410,6 +410,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
> *dma_buf,
> >       .vmap = drm_gem_dmabuf_vmap,
> >       .vunmap = drm_gem_dmabuf_vunmap,
> >  };
> > +EXPORT_SYMBOL(drm_gem_prime_dmabuf_ops);
> >
> >  /**
> >   * DOC: PRIME Helpers
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 6105c05..e0ea8f8 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -761,6 +761,8 @@ static inline int drm_debugfs_remove_files(const
> struct drm_info_list *files,
> >
> >  struct dma_buf_export_info;
> >
> > +extern const struct dma_buf_ops drm_gem_prime_dmabuf_ops;
> > +
> >  extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> >                                           struct drm_gem_object *obj,
> >                                           int flags);
> >
> 
> 
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/12] drm: export drm_gem_prime_dmabuf_ops

[Christian König]

Am 04.07.2017 um 08:26 schrieb Daniel Vetter:
> On Tue, Jul 04, 2017 at 02:55:00PM +1000, Dave Airlie wrote:
>> On 4 Jul. 2017 11:23, "Michel Dänzer" <michel@daenzer.net> wrote:
>>
>>
>> Adding the dri-devel list, since this is a core DRM patch.
>>
>>
>> On 04/07/17 06:11 AM, Felix Kuehling wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> This allows drivers to check if a DMA-buf contains a GEM object or not.
>>
>> Please use an accessor function. I doubt it'll be a fast path.
> I think even better if that helper checks against the driver instance too,
> since that's really the answer you want drm_prime_try_cast_to_gem_bo or
> similar, which will return NULL if it's not a prime dma-buf or not one for
> the current driver instance. Feel free to improve the naming :-)

Yeah, agree with that approach.

Additional to that please split this set from the patches which change 
the hardware config.

That should make it much easier to review,
Christian.

> -Daniel
>
>> Dave.
>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_prime.c | 3 ++-
>>>   include/drm/drmP.h          | 2 ++
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 25aa455..5cb4fd9 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -396,7 +396,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
>> *dma_buf,
>>>        return dev->driver->gem_prime_mmap(obj, vma);
>>>   }
>>>
>>> -static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>>> +const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>>>        .attach = drm_gem_map_attach,
>>>        .detach = drm_gem_map_detach,
>>>        .map_dma_buf = drm_gem_map_dma_buf,
>>> @@ -410,6 +410,7 @@ static int drm_gem_dmabuf_mmap(struct dma_buf
>> *dma_buf,
>>>        .vmap = drm_gem_dmabuf_vmap,
>>>        .vunmap = drm_gem_dmabuf_vunmap,
>>>   };
>>> +EXPORT_SYMBOL(drm_gem_prime_dmabuf_ops);
>>>
>>>   /**
>>>    * DOC: PRIME Helpers
>>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>>> index 6105c05..e0ea8f8 100644
>>> --- a/include/drm/drmP.h
>>> +++ b/include/drm/drmP.h
>>> @@ -761,6 +761,8 @@ static inline int drm_debugfs_remove_files(const
>> struct drm_info_list *files,
>>>   struct dma_buf_export_info;
>>>
>>> +extern const struct dma_buf_ops drm_gem_prime_dmabuf_ops;
>>> +
>>>   extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>>>                                            struct drm_gem_object *obj,
>>>                                            int flags);
>>>
>>
>> --
>> Earthling Michel Dänzer               |               http://www.amd.com
>> Libre software enthusiast             |             Mesa and X developer
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

[PATCH 08/12] drm/amdgpu: disallow foreign BOs for UVD/VCE

[Felix Kuehling]

From: Christian König <christian.koenig@amd.com>

They don't support VM mode yet.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 82131d7..24035e4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1343,7 +1343,8 @@ struct amdgpu_bo_va_mapping *
 		struct amdgpu_bo_list_entry *lobj;
 
 		lobj = &parser->bo_list->array[i];
-		if (!lobj->bo_va)
+		if (!lobj->bo_va ||
+		    amdgpu_ttm_adev(lobj->bo_va->bo->tbo.bdev) != parser->adev)
 			continue;
 
 		list_for_each_entry(mapping, &lobj->bo_va->valids, list) {
-- 
1.9.1

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

Re: [PATCH 08/12] drm/amdgpu: disallow foreign BOs for UVD/VCE

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> They don't support VM mode yet.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

This could probably be refined since newer asics support VM for MM
engines.  Maybe add a comment to that effect?  I would add a comment
in general either way.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 82131d7..24035e4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1343,7 +1343,8 @@ struct amdgpu_bo_va_mapping *
>                 struct amdgpu_bo_list_entry *lobj;
>
>                 lobj = &parser->bo_list->array[i];
> -               if (!lobj->bo_va)
> +               if (!lobj->bo_va ||
> +                   amdgpu_ttm_adev(lobj->bo_va->bo->tbo.bdev) != parser->adev)
>                         continue;
>
>                 list_for_each_entry(mapping, &lobj->bo_va->valids, list) {
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 08/12] drm/amdgpu: disallow foreign BOs for UVD/VCE

[Felix Kuehling]

On 17-07-03 10:55 PM, Alex Deucher wrote:
> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> They don't support VM mode yet.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> This could probably be refined since newer asics support VM for MM
> engines.  Maybe add a comment to that effect?  I would add a comment
> in general either way.
As I understand it, the code doesn't specifically check for UVD or VCE.
It just looks at whether the BO list entry has a virtual address
mapping. If on newer chips user mode creates a virtual address mapping
for UVD/VCE, they should be able to use foreign BOs just fine.

So I think, all I need to change is the commit message.

Regards,
  Felix

>
> Alex
>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 82131d7..24035e4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1343,7 +1343,8 @@ struct amdgpu_bo_va_mapping *
>>                 struct amdgpu_bo_list_entry *lobj;
>>
>>                 lobj = &parser->bo_list->array[i];
>> -               if (!lobj->bo_va)
>> +               if (!lobj->bo_va ||
>> +                   amdgpu_ttm_adev(lobj->bo_va->bo->tbo.bdev) != parser->adev)
>>                         continue;
>>
>>                 list_for_each_entry(mapping, &lobj->bo_va->valids, list) {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 08/12] drm/amdgpu: disallow foreign BOs for UVD/VCE

[Christian König]

Am 12.07.2017 um 07:06 schrieb Felix Kuehling:
> On 17-07-03 10:55 PM, Alex Deucher wrote:
>> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> They don't support VM mode yet.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> This could probably be refined since newer asics support VM for MM
>> engines.  Maybe add a comment to that effect?  I would add a comment
>> in general either way.
> As I understand it, the code doesn't specifically check for UVD or VCE.
> It just looks at whether the BO list entry has a virtual address
> mapping. If on newer chips user mode creates a virtual address mapping
> for UVD/VCE, they should be able to use foreign BOs just fine.
>
> So I think, all I need to change is the commit message.

Yeah, correct. The code is called for all rings which need to access 
it's buffers using VMID0, at the time of writing that was just UVD/VCE.

Sorry, haven't seen Alex mail earlier, other I would have already 
corrected that.

Regards,
Christian.

>
> Regards,
>    Felix
>
>> Alex
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 82131d7..24035e4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1343,7 +1343,8 @@ struct amdgpu_bo_va_mapping *
>>>                  struct amdgpu_bo_list_entry *lobj;
>>>
>>>                  lobj = &parser->bo_list->array[i];
>>> -               if (!lobj->bo_va)
>>> +               if (!lobj->bo_va ||
>>> +                   amdgpu_ttm_adev(lobj->bo_va->bo->tbo.bdev) != parser->adev)
>>>                          continue;
>>>
>>>                  list_for_each_entry(mapping, &lobj->bo_va->valids, list) {
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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

[PATCH 09/12] drm/amdgpu: disallow foreign BOs in the display path

[Felix Kuehling]

From: Christian König <christian.koenig@amd.com>

Pinning them in other devices VRAM would obviously not work.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 3341c34..bd6b0dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -180,6 +180,12 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 	obj = new_amdgpu_fb->obj;
 	new_abo = gem_to_amdgpu_bo(obj);
 
+	if (amdgpu_ttm_adev(new_abo->tbo.bdev) != adev) {
+		DRM_ERROR("Foreign BOs not allowed in the display engine\n");
+		r = -EINVAL;
+		goto cleanup;
+	}
+
 	/* pin the new buffer */
 	r = amdgpu_bo_reserve(new_abo, false);
 	if (unlikely(r != 0)) {
-- 
1.9.1

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

Re: [PATCH 09/12] drm/amdgpu: disallow foreign BOs in the display path

[Alex Deucher]

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Pinning them in other devices VRAM would obviously not work.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Do we need to check in the DC (amdgpu_dm_types.c) and the dce
(dce_v*_0.c) code as well?

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 3341c34..bd6b0dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -180,6 +180,12 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>         obj = new_amdgpu_fb->obj;
>         new_abo = gem_to_amdgpu_bo(obj);
>
> +       if (amdgpu_ttm_adev(new_abo->tbo.bdev) != adev) {
> +               DRM_ERROR("Foreign BOs not allowed in the display engine\n");
> +               r = -EINVAL;
> +               goto cleanup;
> +       }
> +
>         /* pin the new buffer */
>         r = amdgpu_bo_reserve(new_abo, false);
>         if (unlikely(r != 0)) {
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 09/12] drm/amdgpu: disallow foreign BOs in the display path

[Felix Kuehling]

On 17-07-06 05:13 PM, Alex Deucher wrote:
> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> Pinning them in other devices VRAM would obviously not work.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Do we need to check in the DC (amdgpu_dm_types.c) and the dce
> (dce_v*_0.c) code as well?

Probably. I have it as a TODO on my separate P2P patch series.

Regards,
  Felix

>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> index 3341c34..bd6b0dc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> @@ -180,6 +180,12 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>>         obj = new_amdgpu_fb->obj;
>>         new_abo = gem_to_amdgpu_bo(obj);
>>
>> +       if (amdgpu_ttm_adev(new_abo->tbo.bdev) != adev) {
>> +               DRM_ERROR("Foreign BOs not allowed in the display engine\n");
>> +               r = -EINVAL;
>> +               goto cleanup;
>> +       }
>> +
>>         /* pin the new buffer */
>>         r = amdgpu_bo_reserve(new_abo, false);
>>         if (unlikely(r != 0)) {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[PATCH 10/12] drm/amdgpu: separate BO from GEM object

[Felix Kuehling]

From: Christian König <christian.koenig@amd.com>

This allows us to have multiple GEM objects for one BO.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 12 +++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 41 +++++++++++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  7 +----
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c  | 20 ++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 17 +++++++++++--
 5 files changed, 77 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 2129fbb..f3d99cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -414,6 +414,12 @@ struct amdgpu_bo_va {
 
 #define AMDGPU_GEM_DOMAIN_MAX		0x3
 
+struct amdgpu_gem_object {
+	struct drm_gem_object		base;
+	struct list_head		list;
+	struct amdgpu_bo		*bo;
+};
+
 struct amdgpu_bo {
 	/* Protected by tbo.reserved */
 	u32				prefered_domains;
@@ -430,12 +436,14 @@ struct amdgpu_bo {
 	void				*metadata;
 	u32				metadata_size;
 	unsigned			prime_shared_count;
+	/* GEM objects refereing to this BO */
+	struct list_head	gem_objects;
+
 	/* list of all virtual address to which this bo
 	 * is associated to
 	 */
 	struct list_head		va;
 	/* Constant after initialization */
-	struct drm_gem_object		gem_base;
 	struct amdgpu_bo		*parent;
 	struct amdgpu_bo		*shadow;
 
@@ -444,7 +452,7 @@ struct amdgpu_bo {
 	struct list_head		mn_list;
 	struct list_head		shadow_list;
 };
-#define gem_to_amdgpu_bo(gobj) container_of((gobj), struct amdgpu_bo, gem_base)
+#define gem_to_amdgpu_bo(gobj) container_of((gobj), struct amdgpu_gem_object, base)->bo
 
 void amdgpu_gem_object_free(struct drm_gem_object *obj);
 int amdgpu_gem_object_open(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 96c4493..f7e9bdf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -33,14 +33,20 @@
 
 void amdgpu_gem_object_free(struct drm_gem_object *gobj)
 {
-	struct amdgpu_bo *robj = gem_to_amdgpu_bo(gobj);
+	struct amdgpu_gem_object *aobj;
 
-	if (robj) {
-		if (robj->gem_base.import_attach)
-			drm_prime_gem_destroy(&robj->gem_base, robj->tbo.sg);
-		amdgpu_mn_unregister(robj);
-		amdgpu_bo_unref(&robj);
-	}
+	aobj = container_of((gobj), struct amdgpu_gem_object, base);
+	if (aobj->base.import_attach)
+		drm_prime_gem_destroy(&aobj->base, aobj->bo->tbo.sg);
+
+	ww_mutex_lock(&aobj->bo->tbo.resv->lock, NULL);
+	list_del(&aobj->list);
+	ww_mutex_unlock(&aobj->bo->tbo.resv->lock);
+
+	amdgpu_mn_unregister(aobj->bo);
+	amdgpu_bo_unref(&aobj->bo);
+	drm_gem_object_release(&aobj->base);
+	kfree(aobj);
 }
 
 int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
@@ -49,6 +55,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 				struct drm_gem_object **obj)
 {
 	struct amdgpu_bo *robj;
+	struct amdgpu_gem_object *gobj;
 	unsigned long max_size;
 	int r;
 
@@ -83,7 +90,23 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 		}
 		return r;
 	}
-	*obj = &robj->gem_base;
+
+	gobj = kzalloc(sizeof(struct amdgpu_gem_object), GFP_KERNEL);
+	if (unlikely(!gobj)) {
+		amdgpu_bo_unref(&robj);
+		return -ENOMEM;
+	}
+
+	r = drm_gem_object_init(adev->ddev, &gobj->base, amdgpu_bo_size(robj));
+	if (unlikely(r)) {
+		kfree(gobj);
+		amdgpu_bo_unref(&robj);
+		return r;
+	}
+
+	list_add(&gobj->list, &robj->gem_objects);
+	gobj->bo = robj;
+	*obj = &gobj->base;
 
 	return 0;
 }
@@ -703,7 +726,7 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 		struct drm_amdgpu_gem_create_in info;
 		void __user *out = (void __user *)(uintptr_t)args->value;
 
-		info.bo_size = robj->gem_base.size;
+		info.bo_size = amdgpu_bo_size(robj);
 		info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
 		info.domains = robj->prefered_domains;
 		info.domain_flags = robj->flags;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index c34cf2c..44b7e71 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -95,7 +95,6 @@ static void amdgpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 
 	amdgpu_update_memory_usage(adev, &bo->tbo.mem, NULL);
 
-	drm_gem_object_release(&bo->gem_base);
 	amdgpu_bo_unref(&bo->parent);
 	if (!list_empty(&bo->shadow_list)) {
 		mutex_lock(&adev->shadow_list_lock);
@@ -344,13 +343,9 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 	bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
 	if (bo == NULL)
 		return -ENOMEM;
-	r = drm_gem_object_init(adev->ddev, &bo->gem_base, size);
-	if (unlikely(r)) {
-		kfree(bo);
-		return r;
-	}
 	INIT_LIST_HEAD(&bo->shadow_list);
 	INIT_LIST_HEAD(&bo->va);
+	INIT_LIST_HEAD(&bo->gem_objects);
 	bo->prefered_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
 					 AMDGPU_GEM_DOMAIN_GTT |
 					 AMDGPU_GEM_DOMAIN_CPU |
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 6bdc866..b9425ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -65,6 +65,7 @@ struct drm_gem_object *
 	struct reservation_object *resv = attach->dmabuf->resv;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_bo *bo;
+	struct amdgpu_gem_object *gobj;
 	int ret;
 
 	ww_mutex_lock(&resv->lock, NULL);
@@ -75,7 +76,24 @@ struct drm_gem_object *
 		return ERR_PTR(ret);
 
 	bo->prime_shared_count = 1;
-	return &bo->gem_base;
+
+	gobj = kzalloc(sizeof(struct amdgpu_gem_object), GFP_KERNEL);
+	if (unlikely(!gobj)) {
+		amdgpu_bo_unref(&bo);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ret = drm_gem_object_init(adev->ddev, &gobj->base, amdgpu_bo_size(bo));
+	if (unlikely(ret)) {
+		kfree(gobj);
+		amdgpu_bo_unref(&bo);
+		return ERR_PTR(ret);
+	}
+
+	list_add(&gobj->list, &bo->gem_objects);
+	gobj->bo = amdgpu_bo_ref(bo);
+
+	return &gobj->base;
 }
 
 int amdgpu_gem_prime_pin(struct drm_gem_object *obj)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3f927c2..d0198f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -235,11 +235,24 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
 static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp)
 {
 	struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
+	struct drm_file *file_priv = filp->private_data;
+	struct amdgpu_gem_object *gobj;
 
 	if (amdgpu_ttm_tt_get_usermm(bo->ttm))
 		return -EPERM;
-	return drm_vma_node_verify_access(&abo->gem_base.vma_node,
-					  filp->private_data);
+
+	ww_mutex_lock(&abo->tbo.resv->lock, NULL);
+	list_for_each_entry(gobj, &abo->gem_objects, list) {
+		if (gobj->base.dev != file_priv->minor->dev)
+			continue;
+
+		ww_mutex_unlock(&abo->tbo.resv->lock);
+		return drm_vma_node_verify_access(&gobj->base.vma_node,
+						  file_priv);
+	}
+	ww_mutex_unlock(&abo->tbo.resv->lock);
+
+	return -EPERM;
 }
 
 static void amdgpu_move_null(struct ttm_buffer_object *bo,
-- 
1.9.1

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

[PATCH 11/12] drm/amdgpu: handle foreign BOs in the VM mapping

[Felix Kuehling]

From: Amber Lin <Amber.Lin@amd.com>

Set the system bit for foreign BO mappings and use the remote VRAM
BAR address as the VRAM base offset.

Signed-off-by: Amber Lin <Amber.Lin@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1d1810d..5f08e81 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
 				      dma_addr_t *pages_addr,
 				      struct amdgpu_vm *vm,
 				      struct amdgpu_bo_va_mapping *mapping,
+				      uint64_t vram_base_offset,
 				      uint64_t flags,
 				      struct drm_mm_node *nodes,
 				      struct dma_fence **fence)
@@ -1640,7 +1641,7 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
 				max_entries = min(max_entries, 16ull * 1024ull);
 			addr = 0;
 		} else if (flags & AMDGPU_PTE_VALID) {
-			addr += adev->vm_manager.vram_base_offset;
+			addr += vram_base_offset;
 		}
 		addr += pfn << PAGE_SHIFT;
 
@@ -1685,6 +1686,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 	struct ttm_mem_reg *mem;
 	struct drm_mm_node *nodes;
 	struct dma_fence *exclusive;
+	uint64_t vram_base_offset = adev->vm_manager.vram_base_offset;
+	struct amdgpu_device *bo_adev;
 	int r;
 
 	if (clear || !bo_va->bo) {
@@ -1706,9 +1709,15 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 
 	if (bo_va->bo) {
 		flags = amdgpu_ttm_tt_pte_flags(adev, bo_va->bo->tbo.ttm, mem);
+		bo_adev = amdgpu_ttm_adev(bo_va->bo->tbo.bdev);
 		gtt_flags = (amdgpu_ttm_is_bound(bo_va->bo->tbo.ttm) &&
-			adev == amdgpu_ttm_adev(bo_va->bo->tbo.bdev)) ?
+			adev == bo_adev) ?
 			flags : 0;
+		if (mem && mem->mem_type == TTM_PL_VRAM &&
+			adev != bo_adev) {
+			flags |= AMDGPU_PTE_SYSTEM;
+			vram_base_offset = bo_adev->mc.aper_base;
+		}
 	} else {
 		flags = 0x0;
 		gtt_flags = ~0x0;
@@ -1722,8 +1731,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 	list_for_each_entry(mapping, &bo_va->invalids, list) {
 		r = amdgpu_vm_bo_split_mapping(adev, exclusive,
 					       gtt_flags, pages_addr, vm,
-					       mapping, flags, nodes,
-					       &bo_va->last_pt_update);
+					       mapping, vram_base_offset, flags,
+					       nodes, &bo_va->last_pt_update);
 		if (r)
 			return r;
 	}
-- 
1.9.1

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

[PATCH 12/12] drm/amdgpu: enable foreign DMA-buf objects

[Felix Kuehling]

From: Christian König <christian.koenig@amd.com>

We should be able to handle BOs from other instances as well.

Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 59 +++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f3d99cb..18b2c28 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -468,6 +468,8 @@ struct drm_gem_object *
 struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 					struct drm_gem_object *gobj,
 					int flags);
+struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
+					       struct dma_buf *dma_buf);
 int amdgpu_gem_prime_pin(struct drm_gem_object *obj);
 void amdgpu_gem_prime_unpin(struct drm_gem_object *obj);
 struct reservation_object *amdgpu_gem_prime_res_obj(struct drm_gem_object *);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 02cf24e..df78a3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -813,7 +813,7 @@ long amdgpu_drm_ioctl(struct file *filp,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export = amdgpu_gem_prime_export,
-	.gem_prime_import = drm_gem_prime_import,
+	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_pin = amdgpu_gem_prime_pin,
 	.gem_prime_unpin = amdgpu_gem_prime_unpin,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index b9425ed..9f7fae8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -159,3 +159,62 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 
 	return drm_gem_prime_export(dev, gobj, flags);
 }
+
+static struct drm_gem_object *
+amdgpu_gem_prime_foreign_bo(struct amdgpu_device *adev, struct amdgpu_bo *bo)
+{
+	struct amdgpu_gem_object *gobj;
+	int r;
+
+	ww_mutex_lock(&bo->tbo.resv->lock, NULL);
+
+	list_for_each_entry(gobj, &bo->gem_objects, list) {
+		if (gobj->base.dev != adev->ddev)
+			continue;
+
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		drm_gem_object_reference(&gobj->base);
+		return &gobj->base;
+	}
+
+
+	gobj = kzalloc(sizeof(struct amdgpu_gem_object), GFP_KERNEL);
+	if (unlikely(!gobj)) {
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	r = drm_gem_object_init(adev->ddev, &gobj->base, amdgpu_bo_size(bo));
+	if (unlikely(r)) {
+		kfree(gobj);
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		return ERR_PTR(r);
+	}
+
+	list_add(&gobj->list, &bo->gem_objects);
+	gobj->bo = amdgpu_bo_ref(bo);
+	bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+
+	ww_mutex_unlock(&bo->tbo.resv->lock);
+
+	return &gobj->base;
+}
+
+struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
+					       struct dma_buf *dma_buf)
+{
+	struct amdgpu_device *adev = dev->dev_private;
+
+	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
+		struct drm_gem_object *obj = dma_buf->priv;
+
+		if (obj->dev != dev && obj->dev->driver == dev->driver) {
+			/* It's a amdgpu_bo from a different driver instance */
+			struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+			return amdgpu_gem_prime_foreign_bo(adev, bo);
+		}
+	}
+
+	return drm_gem_prime_import(dev, dma_buf);
+}
-- 
1.9.1

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

Re: [PATCH 12/12] drm/amdgpu: enable foreign DMA-buf objects

[Christian König]

Am 03.07.2017 um 23:11 schrieb Felix Kuehling:
> From: Christian König <christian.koenig@amd.com>
>
> We should be able to handle BOs from other instances as well.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 59 +++++++++++++++++++++++++++++++
>   3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f3d99cb..18b2c28 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -468,6 +468,8 @@ struct drm_gem_object *
>   struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
>   					struct drm_gem_object *gobj,
>   					int flags);
> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
> +					       struct dma_buf *dma_buf);
>   int amdgpu_gem_prime_pin(struct drm_gem_object *obj);
>   void amdgpu_gem_prime_unpin(struct drm_gem_object *obj);
>   struct reservation_object *amdgpu_gem_prime_res_obj(struct drm_gem_object *);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 02cf24e..df78a3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -813,7 +813,7 @@ long amdgpu_drm_ioctl(struct file *filp,
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>   	.gem_prime_export = amdgpu_gem_prime_export,
> -	.gem_prime_import = drm_gem_prime_import,
> +	.gem_prime_import = amdgpu_gem_prime_import,
>   	.gem_prime_pin = amdgpu_gem_prime_pin,
>   	.gem_prime_unpin = amdgpu_gem_prime_unpin,
>   	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index b9425ed..9f7fae8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -159,3 +159,62 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
>   
>   	return drm_gem_prime_export(dev, gobj, flags);
>   }
> +
> +static struct drm_gem_object *
> +amdgpu_gem_prime_foreign_bo(struct amdgpu_device *adev, struct amdgpu_bo *bo)
> +{
> +	struct amdgpu_gem_object *gobj;
> +	int r;
> +
> +	ww_mutex_lock(&bo->tbo.resv->lock, NULL);
> +
> +	list_for_each_entry(gobj, &bo->gem_objects, list) {
> +		if (gobj->base.dev != adev->ddev)
> +			continue;
> +
> +		ww_mutex_unlock(&bo->tbo.resv->lock);
> +		drm_gem_object_reference(&gobj->base);
> +		return &gobj->base;
> +	}
> +
> +
> +	gobj = kzalloc(sizeof(struct amdgpu_gem_object), GFP_KERNEL);
> +	if (unlikely(!gobj)) {
> +		ww_mutex_unlock(&bo->tbo.resv->lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	r = drm_gem_object_init(adev->ddev, &gobj->base, amdgpu_bo_size(bo));
> +	if (unlikely(r)) {
> +		kfree(gobj);
> +		ww_mutex_unlock(&bo->tbo.resv->lock);
> +		return ERR_PTR(r);
> +	}
> +
> +	list_add(&gobj->list, &bo->gem_objects);
> +	gobj->bo = amdgpu_bo_ref(bo);
> +	bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;

It's a bit more tricker than that. IIRC my original patch limited the BO 
to GTT space as well.

VRAM peer to peer access doesn't work with most PCIe chipsets.

At bare minimum we need to put this behind a config option or add a 
white list for the chipset or only enable it if "pci=pcie_bus_peer2peer" 
is set or something like this.

BTW: If you modify a patch as severely as that please add your 
Signed-of-by line as well.

Regards,
Christian.

> +
> +	ww_mutex_unlock(&bo->tbo.resv->lock);
> +
> +	return &gobj->base;
> +}
> +
> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
> +					       struct dma_buf *dma_buf)
> +{
> +	struct amdgpu_device *adev = dev->dev_private;
> +
> +	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
> +		struct drm_gem_object *obj = dma_buf->priv;
> +
> +		if (obj->dev != dev && obj->dev->driver == dev->driver) {
> +			/* It's a amdgpu_bo from a different driver instance */
> +			struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +			return amdgpu_gem_prime_foreign_bo(adev, bo);
> +		}
> +	}
> +
> +	return drm_gem_prime_import(dev, dma_buf);
> +}


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

Re: [PATCH 12/12] drm/amdgpu: enable foreign DMA-buf objects

[Felix Kuehling]

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


On 17-07-04 03:32 AM, Christian König wrote:
> Am 03.07.2017 um 23:11 schrieb Felix Kuehling:
>> +
>> +    list_add(&gobj->list, &bo->gem_objects);
>> +    gobj->bo = amdgpu_bo_ref(bo);
>> +    bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>
> It's a bit more tricker than that. IIRC my original patch limited the
> BO to GTT space as well.

I dug out your original patch for reference (attached), which was
originally applied on a 4.1-based KFD staging branch. Your original
patch series also included a change for VRAM VM mappings with the system
bit (also attached). So your original intention was clearly to also
allow VRAM P2P access. The VM patch didn't apply after some VM changes
(probably related to the vram manager) and was later replaced by Amber's
patch.

>
> VRAM peer to peer access doesn't work with most PCIe chipsets.
>
> At bare minimum we need to put this behind a config option or add a
> white list for the chipset or only enable it if
> "pci=pcie_bus_peer2peer" is set or something like this.

Well we're using it without any special pci= flags.
pci=pcie_bus_peer2peer can reduce performance, so we should not require
it if it's not needed on all systems.

There are other issues that can prevent P2P access between some pairs of
devices. For example on Intel dual-socket boards the QPI link between
the sockets doesn't work for P2P traffic. So P2P only works between
devices connected to the same socket.

I think it's impractical to check all those chipset-specific limitations
at this level. Importing and mapping a foreign BO should be no problem
either way. If remote access is limited, that's something the
application can figure out on its own. In case of KFD, this is done
based on IO-link topology information.

Regards,
  Felix


>
> BTW: If you modify a patch as severely as that please add your
> Signed-of-by line as well.
>
> Regards,
> Christian.
>
>> +
>> +    ww_mutex_unlock(&bo->tbo.resv->lock);
>> +
>> +    return &gobj->base;
>> +}
>> +
>> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>> +                           struct dma_buf *dma_buf)
>> +{
>> +    struct amdgpu_device *adev = dev->dev_private;
>> +
>> +    if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>> +        struct drm_gem_object *obj = dma_buf->priv;
>> +
>> +        if (obj->dev != dev && obj->dev->driver == dev->driver) {
>> +            /* It's a amdgpu_bo from a different driver instance */
>> +            struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>> +
>> +            return amdgpu_gem_prime_foreign_bo(adev, bo);
>> +        }
>> +    }
>> +
>> +    return drm_gem_prime_import(dev, dma_buf);
>> +}
>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-amdgpu-enable-foreign-DMA-buf-objects.patch --]
[-- Type: text/x-patch; name="0001-drm-amdgpu-enable-foreign-DMA-buf-objects.patch", Size: 3998 bytes --]

>From 9c9b304eaf22730f934d4e60c9eb02fbd0c52613 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig-5C7GfCeVMHo@public.gmane.org>
Date: Wed, 2 Dec 2015 15:40:55 +0100
Subject: [PATCH] drm/amdgpu: enable foreign DMA-buf objects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We should be able to handle BOs from other instances as well.

Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 59 +++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 6d5c8980..816179d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -639,6 +639,8 @@ struct drm_gem_object *amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 					struct drm_gem_object *gobj,
 					int flags);
+struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
+					       struct dma_buf *dma_buf);
 int amdgpu_gem_prime_pin(struct drm_gem_object *obj);
 void amdgpu_gem_prime_unpin(struct drm_gem_object *obj);
 struct reservation_object *amdgpu_gem_prime_res_obj(struct drm_gem_object *);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 360631d..1a185d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -514,7 +514,7 @@ long amdgpu_drm_ioctl(struct file *filp,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export = amdgpu_gem_prime_export,
-	.gem_prime_import = drm_gem_prime_import,
+	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_pin = amdgpu_gem_prime_pin,
 	.gem_prime_unpin = amdgpu_gem_prime_unpin,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 649364f..d7c0f9f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -143,3 +143,62 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 
 	return drm_gem_prime_export(dev, gobj, flags);
 }
+
+static struct drm_gem_object *
+amdgpu_gem_prime_foreign_bo(struct amdgpu_device *adev, struct amdgpu_bo *bo)
+{
+	struct amdgpu_gem_object *gobj;
+	int r;
+
+	ww_mutex_lock(&bo->tbo.resv->lock, NULL);
+
+	list_for_each_entry(gobj, &bo->gem_objects, list) {
+		if (gobj->base.dev != adev->ddev)
+			continue;
+
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		drm_gem_object_reference(&gobj->base);
+		return &gobj->base;
+	}
+
+
+	gobj = kzalloc(sizeof(struct amdgpu_gem_object), GFP_KERNEL);
+	if (unlikely(!gobj)) {
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	r = drm_gem_object_init(adev->ddev, &gobj->base, amdgpu_bo_size(bo));
+	if (unlikely(r)) {
+		kfree(gobj);
+		ww_mutex_unlock(&bo->tbo.resv->lock);
+		return ERR_PTR(r);
+	}
+
+	list_add(&gobj->list, &bo->gem_objects);
+	gobj->bo = amdgpu_bo_ref(bo);
+	bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+
+	ww_mutex_unlock(&bo->tbo.resv->lock);
+
+	return &gobj->base;
+}
+
+struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
+					       struct dma_buf *dma_buf)
+{
+	struct amdgpu_device *adev = dev->dev_private;
+
+	if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
+		struct drm_gem_object *obj = dma_buf->priv;
+
+		if (obj->dev != dev && obj->dev->driver == dev->driver) {
+			/* It's a amdgpu_bo from a different driver instance */
+			struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+			return amdgpu_gem_prime_foreign_bo(adev, bo);
+		}
+	}
+
+	return drm_gem_prime_import(dev, dma_buf);
+}
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-drm-amdgpu-handle-foreign-BOs-in-the-VM-mapping-code.patch --]
[-- Type: text/x-patch; name="0001-drm-amdgpu-handle-foreign-BOs-in-the-VM-mapping-code.patch", Size: 1938 bytes --]

>From fc703df5a630852a70656dbd4842bcacb0b4cf69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig-5C7GfCeVMHo@public.gmane.org>
Date: Mon, 30 Nov 2015 14:30:08 +0100
Subject: [PATCH] drm/amdgpu: handle foreign BOs in the VM mapping code v2
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

That should allow us using foreign BOs in VRAM.

v2: use the correct device for the PTE creation.

Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 47e9f41..a02889c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -843,12 +843,15 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			struct ttm_mem_reg *mem)
 {
 	struct amdgpu_vm *vm = bo_va->vm;
+	struct amdgpu_bo *bo = bo_va->bo;
 	struct amdgpu_bo_va_mapping *mapping;
 	struct amdgpu_gart *gtt = NULL;
 	uint32_t flags;
 	uint64_t addr;
 	int r;
 
+	flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem);
+
 	if (mem) {
 		addr = (u64)mem->start << PAGE_SHIFT;
 		switch (mem->mem_type) {
@@ -857,7 +860,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			break;
 
 		case TTM_PL_VRAM:
-			addr += adev->vm_manager.vram_base_offset;
+			if (bo->adev != adev) {
+				addr += bo->adev->mc.aper_base;
+				flags |= AMDGPU_PTE_SYSTEM;
+			} else
+				addr += adev->vm_manager.vram_base_offset;
 			break;
 
 		default:
@@ -867,8 +874,6 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 		addr = 0;
 	}
 
-	flags = amdgpu_ttm_tt_pte_flags(adev, bo_va->bo->tbo.ttm, mem);
-
 	spin_lock(&vm->status_lock);
 	if (!list_empty(&bo_va->vm_status))
 		list_splice_init(&bo_va->valids, &bo_va->invalids);
-- 
1.9.1


[-- Attachment #4: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 12/12] drm/amdgpu: enable foreign DMA-buf objects

[Christian König]

Am 04.07.2017 um 17:56 schrieb Felix Kuehling:
> On 17-07-04 03:32 AM, Christian König wrote:
>> Am 03.07.2017 um 23:11 schrieb Felix Kuehling:
>>> +
>>> +    list_add(&gobj->list, &bo->gem_objects);
>>> +    gobj->bo = amdgpu_bo_ref(bo);
>>> +    bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>> It's a bit more tricker than that. IIRC my original patch limited the
>> BO to GTT space as well.
> I dug out your original patch for reference (attached), which was
> originally applied on a 4.1-based KFD staging branch. Your original
> patch series also included a change for VRAM VM mappings with the system
> bit (also attached). So your original intention was clearly to also
> allow VRAM P2P access. The VM patch didn't apply after some VM changes
> (probably related to the vram manager) and was later replaced by Amber's
> patch.

Ok in this case my memory fooled me.

>> VRAM peer to peer access doesn't work with most PCIe chipsets.
>>
>> At bare minimum we need to put this behind a config option or add a
>> white list for the chipset or only enable it if
>> "pci=pcie_bus_peer2peer" is set or something like this.
> Well we're using it without any special pci= flags.
> pci=pcie_bus_peer2peer can reduce performance, so we should not require
> it if it's not needed on all systems.
>
> There are other issues that can prevent P2P access between some pairs of
> devices. For example on Intel dual-socket boards the QPI link between
> the sockets doesn't work for P2P traffic. So P2P only works between
> devices connected to the same socket.
>
> I think it's impractical to check all those chipset-specific limitations
> at this level.

Seriously? Sorry, but that approach is a clear no-go.

Long story short all those cases must cleanly be handled before this 
patch series can land upstream (or even be in any hybrid release).

> Importing and mapping a foreign BO should be no problem
> either way. If remote access is limited, that's something the
> application can figure out on its own. In case of KFD, this is done
> based on IO-link topology information.

I'm pretty sure that the patch as is would break A+A laptops, so pushing 
it to any branch outside some KFD testing environment is a clear NAK 
from my side.

I would also strongly discourage to use it in a production system until 
those issues are sorted out. This patch series was a technical prove of 
concept, not something we can upstream as is.

What needs to be done is working on a white list for chipsets and/or 
interconnections between PCIe bus systems.

Regards,
Christian.

>
> Regards,
>    Felix
>
>
>> BTW: If you modify a patch as severely as that please add your
>> Signed-of-by line as well.
>>
>> Regards,
>> Christian.
>>
>>> +
>>> +    ww_mutex_unlock(&bo->tbo.resv->lock);
>>> +
>>> +    return &gobj->base;
>>> +}
>>> +
>>> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>>> +                           struct dma_buf *dma_buf)
>>> +{
>>> +    struct amdgpu_device *adev = dev->dev_private;
>>> +
>>> +    if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>>> +        struct drm_gem_object *obj = dma_buf->priv;
>>> +
>>> +        if (obj->dev != dev && obj->dev->driver == dev->driver) {
>>> +            /* It's a amdgpu_bo from a different driver instance */
>>> +            struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>> +
>>> +            return amdgpu_gem_prime_foreign_bo(adev, bo);
>>> +        }
>>> +    }
>>> +
>>> +    return drm_gem_prime_import(dev, dma_buf);
>>> +}
>>

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

Re: [PATCH 12/12] drm/amdgpu: enable foreign DMA-buf objects

[Felix Kuehling]

On 17-07-04 12:39 PM, Christian König wrote:
> Long story short all those cases must cleanly be handled before this
> patch series can land upstream (or even be in any hybrid release). 
FWIW, it's in 17.20.

> I'm pretty sure that the patch as is would break A+A laptops, so
> pushing it to any branch outside some KFD testing environment is a
> clear NAK from my side.

On A+A laptops, one side of the P2P is system memory, so it shouldn't be
a problem.

>
> I would also strongly discourage to use it in a production system
> until those issues are sorted out. This patch series was a technical
> prove of concept, not something we can upstream as is.
>
> What needs to be done is working on a white list for chipsets and/or
> interconnections between PCIe bus systems. 
If I understand your concerns correctly, it's that buffer sharing across
GPUs can be used in amdgpu graphics use cases today, but forces the
memory to be migrated to system memory and shared through scatter-gather
lists. I wasn't aware of such use cases. A+A laptops shouldn't be a
problem because it's not really P2P like I pointed out above.

This patch series improves buffer sharing between GPUs but breaks if the
chipset doesn't support P2P, if there are such use cases today. I'm
still sceptical about that assumption.

So we'd need some conditions to check whether P2P is supported by the
chipset (whitelist, or maybe a config option or module parameter) for
ROCm setups that need P2P. And a working fallback path (the old SG-way)
for systems where P2P is not working.

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

Re: [PATCH 00/12] Patches from amd-kfd-staging

[Felix Kuehling]

I split the P2P patches into a separate series for review. Patches 1-6
from this series should still be valid.

I still need Reviewed-by's for patches 1 and 4.

Patches 2 and 5 are by other authors and reviewed by me. But I'd feel
better to get at least an Acked-by from some amdgpu folks.

Thanks,
  Felix


On 17-07-03 05:11 PM, Felix Kuehling wrote:
> Varios cleaned-up and some squashed patches from amd-kfg-staging
> that are not necessarily KFD-on-dGPU-specific. The intention is to
> minimize differences between amd-kfd-staging and upstream before
> reviewing KFD-specific changes for upstreaming.
>
> Patches 7-12 are a rebased (multiple times) patch series by
> Christian for allowing foreign BO imports for peer-to-peer buffer
> access.
>
> Amber Lin (1):
>   drm/amdgpu: handle foreign BOs in the VM mapping
>
> Christian König (5):
>   drm: export drm_gem_prime_dmabuf_ops
>   drm/amdgpu: disallow foreign BOs for UVD/VCE
>   drm/amdgpu: disallow foreign BOs in the display path
>   drm/amdgpu: separate BO from GEM object
>   drm/amdgpu: enable foreign DMA-buf objects
>
> Felix Kuehling (3):
>   drm/amdgpu: implement vm_operations_struct.access
>   drm/amdgpu: Enable SDMA context switching for CIK
>   drm/amdgpu: Make SDMA phase quantum configurable
>
> Jay Cornwall (1):
>   drm/amdgpu: Send no-retry XNACK for all fault types
>
> Yong Zhao (1):
>   drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd
>
> shaoyunl (1):
>   drm/amdgpu: Enable SDMA_CNTL.ATC_L1_ENABLE for SDMA on CZ
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  15 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |   3 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c  |   7 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |   6 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |   6 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     |  41 ++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |   7 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c   |  79 ++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 147 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |   2 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  17 +++-
>  drivers/gpu/drm/amd/amdgpu/cik.c            |   9 +-
>  drivers/gpu/drm/amd/amdgpu/cik_sdma.c       |  60 ++++++++++++
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c    |   3 +
>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c     |   3 +
>  drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c      |  42 +++++++-
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c      |  34 ++++++-
>  drivers/gpu/drm/drm_prime.c                 |   3 +-
>  include/drm/drmP.h                          |   2 +
>  19 files changed, 446 insertions(+), 40 deletions(-)
>

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