[PATCH 1/3] drm/amdgpu: revert "add new bo flag that indicates BOs don't need fallback (v2)"

[PATCH 1/3] drm/amdgpu: revert "add new bo flag that indicates BOs don't need fallback (v2)"

[Christian König]

This reverts commit 6f51d28bfe8e1a676de5cd877639245bed3cc818.

Makes fallback handling to complicated. This is just a feature for the
GEM interface and shouldn't leak into the core BO create function.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 +----
 include/uapi/drm/amdgpu_drm.h              | 2 --
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 68af2f878bc9..e1756b68a17b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -385,8 +385,7 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
 	    amdgpu_bo_in_cpu_visible_vram(bo))
 		p->bytes_moved_vis += ctx.bytes_moved;
 
-	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains &&
-	    !(bo->flags & AMDGPU_GEM_CREATE_NO_FALLBACK)) {
+	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
 		domain = bo->allowed_domains;
 		goto retry;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 9e23d6f6f3f3..04d6830347ec 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -388,8 +388,6 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
 	INIT_LIST_HEAD(&bo->shadow_list);
 	INIT_LIST_HEAD(&bo->va);
-	bo->preferred_domains = preferred_domains;
-	bo->allowed_domains = allowed_domains;
 
 	bo->flags = flags;
 
@@ -426,8 +424,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
 				 &bo->placement, page_align, &ctx, acc_size,
 				 NULL, resv, &amdgpu_ttm_bo_destroy);
-	if (unlikely(r && r != -ERESTARTSYS) && type == ttm_bo_type_device &&
-	    !(flags & AMDGPU_GEM_CREATE_NO_FALLBACK)) {
+	if (unlikely(r && r != -ERESTARTSYS) && type == ttm_bo_type_device) {
 		if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
 			flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
 			goto retry;
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 80665715e651..0087799962cf 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -98,8 +98,6 @@ extern "C" {
 #define AMDGPU_GEM_CREATE_VM_ALWAYS_VALID	(1 << 6)
 /* Flag that BO sharing will be explicitly synchronized */
 #define AMDGPU_GEM_CREATE_EXPLICIT_SYNC		(1 << 7)
-/* Flag that BO doesn't need fallback */
-#define AMDGPU_GEM_CREATE_NO_FALLBACK		(1 << 8)
 
 struct drm_amdgpu_gem_create_in  {
 	/** the requested memory size */
-- 
2.14.1

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

[PATCH 2/3] drm/amdgpu: revert "Don't change preferred domian when fallback GTT v6"

[Christian König]

This reverts commit 7d1ca1325260a9e9329b10a21e3692e6f188936f.

Makes fallback handling to complicated. This is just a feature for the
GEM interface and shouldn't leak into the core BO create function.

The intended change to preserve the preferred domains is implemented in
a follow up patch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 16 +++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 37 +++++++++++-------------------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 28c2706e48d7..46b9ea4e6103 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -56,11 +56,23 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 		alignment = PAGE_SIZE;
 	}
 
+retry:
 	r = amdgpu_bo_create(adev, size, alignment, initial_domain,
 			     flags, type, resv, &bo);
 	if (r) {
-		DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
-			  size, initial_domain, alignment, r);
+		if (r != -ERESTARTSYS) {
+			if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
+				flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+				goto retry;
+			}
+
+			if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
+				initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
+				goto retry;
+			}
+			DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
+				  size, initial_domain, alignment, r);
+		}
 		return r;
 	}
 	*obj = &bo->gem_base;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 04d6830347ec..6d08cde8443c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -356,7 +356,6 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 	struct amdgpu_bo *bo;
 	unsigned long page_align;
 	size_t acc_size;
-	u32 domains, preferred_domains, allowed_domains;
 	int r;
 
 	page_align = roundup(byte_align, PAGE_SIZE) >> PAGE_SHIFT;
@@ -370,24 +369,22 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 	acc_size = ttm_bo_dma_acc_size(&adev->mman.bdev, size,
 				       sizeof(struct amdgpu_bo));
 
-	preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
-				      AMDGPU_GEM_DOMAIN_GTT |
-				      AMDGPU_GEM_DOMAIN_CPU |
-				      AMDGPU_GEM_DOMAIN_GDS |
-				      AMDGPU_GEM_DOMAIN_GWS |
-				      AMDGPU_GEM_DOMAIN_OA);
-	allowed_domains = preferred_domains;
-	if (type != ttm_bo_type_kernel &&
-	    allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
-		allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
-	domains = preferred_domains;
-retry:
 	bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
 	if (bo == NULL)
 		return -ENOMEM;
 	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
 	INIT_LIST_HEAD(&bo->shadow_list);
 	INIT_LIST_HEAD(&bo->va);
+	bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
+					 AMDGPU_GEM_DOMAIN_GTT |
+					 AMDGPU_GEM_DOMAIN_CPU |
+					 AMDGPU_GEM_DOMAIN_GDS |
+					 AMDGPU_GEM_DOMAIN_GWS |
+					 AMDGPU_GEM_DOMAIN_OA);
+	bo->allowed_domains = bo->preferred_domains;
+	if (type != ttm_bo_type_kernel &&
+	    bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
+		bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
 
 	bo->flags = flags;
 
@@ -420,20 +417,12 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 #endif
 
 	bo->tbo.bdev = &adev->mman.bdev;
-	amdgpu_ttm_placement_from_domain(bo, domains);
+	amdgpu_ttm_placement_from_domain(bo, domain);
+
 	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
 				 &bo->placement, page_align, &ctx, acc_size,
 				 NULL, resv, &amdgpu_ttm_bo_destroy);
-	if (unlikely(r && r != -ERESTARTSYS) && type == ttm_bo_type_device) {
-		if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
-			flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
-			goto retry;
-		} else if (domains != allowed_domains) {
-			domains = allowed_domains;
-			goto retry;
-		}
-	}
-	if (unlikely(r))
+	if (unlikely(r != 0))
 		return r;
 
 	if (adev->gmc.visible_vram_size < adev->gmc.real_vram_size &&
-- 
2.14.1

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

[PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[Christian König]

When GEM needs to fallback to GTT for VRAM BOs we still want the
preferred domain to be untouched so that the BO has a cance to move back
to VRAM in the future.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 46b9ea4e6103..9dc0a190413c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 			     struct reservation_object *resv,
 			     struct drm_gem_object **obj)
 {
+	uint32_t domain = initial_domain;
 	struct amdgpu_bo *bo;
 	int r;
 
@@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 	}
 
 retry:
-	r = amdgpu_bo_create(adev, size, alignment, initial_domain,
+	r = amdgpu_bo_create(adev, size, alignment, domain,
 			     flags, type, resv, &bo);
 	if (r) {
 		if (r != -ERESTARTSYS) {
@@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 				goto retry;
 			}
 
-			if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
-				initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
+			if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
+				domain |= AMDGPU_GEM_DOMAIN_GTT;
 				goto retry;
 			}
 			DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
@@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
 		}
 		return r;
 	}
+
+	bo->preferred_domains = initial_domain;
+	bo->allowed_domains = initial_domain;
+	if (type != ttm_bo_type_kernel &&
+	    bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
+		bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
+
 	*obj = &bo->gem_base;
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 6d08cde8443c..854d18d5cff4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
 	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
 	INIT_LIST_HEAD(&bo->shadow_list);
 	INIT_LIST_HEAD(&bo->va);
-	bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
-					 AMDGPU_GEM_DOMAIN_GTT |
-					 AMDGPU_GEM_DOMAIN_CPU |
-					 AMDGPU_GEM_DOMAIN_GDS |
-					 AMDGPU_GEM_DOMAIN_GWS |
-					 AMDGPU_GEM_DOMAIN_OA);
-	bo->allowed_domains = bo->preferred_domains;
-	if (type != ttm_bo_type_kernel &&
-	    bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
-		bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
-
+	bo->preferred_domains = domain;
+	bo->allowed_domains = domain;
 	bo->flags = flags;
 
 #ifdef CONFIG_X86_32
-- 
2.14.1

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

Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[zhoucm1]

On 2018年04月10日 21:42, Christian König wrote:
> When GEM needs to fallback to GTT for VRAM BOs we still want the
> preferred domain to be untouched so that the BO has a cance to move back
> to VRAM in the future.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>   2 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 46b9ea4e6103..9dc0a190413c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
>   			     struct reservation_object *resv,
>   			     struct drm_gem_object **obj)
>   {
> +	uint32_t domain = initial_domain;
>   	struct amdgpu_bo *bo;
>   	int r;
>   
> @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
>   	}
>   
>   retry:
> -	r = amdgpu_bo_create(adev, size, alignment, initial_domain,
> +	r = amdgpu_bo_create(adev, size, alignment, domain,
>   			     flags, type, resv, &bo);
>   	if (r) {
>   		if (r != -ERESTARTSYS) {
> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
>   				goto retry;
>   			}
>   
> -			if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
> -				initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
> +			if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
> +				domain |= AMDGPU_GEM_DOMAIN_GTT;
>   				goto retry;
>   			}
>   			DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
>   		}
>   		return r;
>   	}
> +
> +	bo->preferred_domains = initial_domain;
> +	bo->allowed_domains = initial_domain;
> +	if (type != ttm_bo_type_kernel &&
> +	    bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
> +		bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
It's an ugly change back after bo creation! Do you think it's better 
than previous?

Alternative way, you can add one parameter to amdgpu_bo_create, directly 
pass preferred_domains and allowed_domains to amdgpu_bo_create.

Regards,
David Zhou
> +
>   	*obj = &bo->gem_base;
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6d08cde8443c..854d18d5cff4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, unsigned long size,
>   	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>   	INIT_LIST_HEAD(&bo->shadow_list);
>   	INIT_LIST_HEAD(&bo->va);
> -	bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
> -					 AMDGPU_GEM_DOMAIN_GTT |
> -					 AMDGPU_GEM_DOMAIN_CPU |
> -					 AMDGPU_GEM_DOMAIN_GDS |
> -					 AMDGPU_GEM_DOMAIN_GWS |
> -					 AMDGPU_GEM_DOMAIN_OA);
> -	bo->allowed_domains = bo->preferred_domains;
> -	if (type != ttm_bo_type_kernel &&
> -	    bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
> -		bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
> -
> +	bo->preferred_domains = domain;
> +	bo->allowed_domains = domain;
>   	bo->flags = flags;
>   
>   #ifdef CONFIG_X86_32

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

Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[Christian König]

Am 11.04.2018 um 04:38 schrieb zhoucm1:
>
>
> On 2018年04月10日 21:42, Christian König wrote:
>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>> preferred domain to be untouched so that the BO has a cance to move back
>> to VRAM in the future.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 46b9ea4e6103..9dc0a190413c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>> *adev, unsigned long size,
>>                    struct reservation_object *resv,
>>                    struct drm_gem_object **obj)
>>   {
>> +    uint32_t domain = initial_domain;
>>       struct amdgpu_bo *bo;
>>       int r;
>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>> *adev, unsigned long size,
>>       }
>>     retry:
>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>                    flags, type, resv, &bo);
>>       if (r) {
>>           if (r != -ERESTARTSYS) {
>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>> *adev, unsigned long size,
>>                   goto retry;
>>               }
>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>                   goto retry;
>>               }
>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, 
>> %d)\n",
>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>> *adev, unsigned long size,
>>           }
>>           return r;
>>       }
>> +
>> +    bo->preferred_domains = initial_domain;
>> +    bo->allowed_domains = initial_domain;
>> +    if (type != ttm_bo_type_kernel &&
>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
> It's an ugly change back after bo creation! Do you think it's better 
> than previous?

Well it's better than moving the fallback handling into the core 
functions and then adding a flag to disable it again because we don't 
want it in the core function.

>
> Alternative way, you can add one parameter to amdgpu_bo_create, 
> directly pass preferred_domains and allowed_domains to amdgpu_bo_create.

Then we would need three parameters, one where to create the BO now and 
two for preferred/allowed domains.

I think that in this case overwriting the placement from the initial 
value looks cleaner to me.

Regards,
Christian.

>
> Regards,
> David Zhou
>> +
>>       *obj = &bo->gem_base;
>>         return 0;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 6d08cde8443c..854d18d5cff4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>> amdgpu_device *adev, unsigned long size,
>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>       INIT_LIST_HEAD(&bo->shadow_list);
>>       INIT_LIST_HEAD(&bo->va);
>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>> -                     AMDGPU_GEM_DOMAIN_GTT |
>> -                     AMDGPU_GEM_DOMAIN_CPU |
>> -                     AMDGPU_GEM_DOMAIN_GDS |
>> -                     AMDGPU_GEM_DOMAIN_GWS |
>> -                     AMDGPU_GEM_DOMAIN_OA);
>> -    bo->allowed_domains = bo->preferred_domains;
>> -    if (type != ttm_bo_type_kernel &&
>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>> -
>> +    bo->preferred_domains = domain;
>> +    bo->allowed_domains = domain;
>>       bo->flags = flags;
>>     #ifdef CONFIG_X86_32
>

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

Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[Christian König]

Am 11.04.2018 um 11:22 schrieb Christian König:
> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>
>>
>> On 2018年04月10日 21:42, Christian König wrote:
>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>> preferred domain to be untouched so that the BO has a cance to move 
>>> back
>>> to VRAM in the future.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 46b9ea4e6103..9dc0a190413c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>> *adev, unsigned long size,
>>>                    struct reservation_object *resv,
>>>                    struct drm_gem_object **obj)
>>>   {
>>> +    uint32_t domain = initial_domain;
>>>       struct amdgpu_bo *bo;
>>>       int r;
>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>> amdgpu_device *adev, unsigned long size,
>>>       }
>>>     retry:
>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>                    flags, type, resv, &bo);
>>>       if (r) {
>>>           if (r != -ERESTARTSYS) {
>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>> *adev, unsigned long size,
>>>                   goto retry;
>>>               }
>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>                   goto retry;
>>>               }
>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, 
>>> %d)\n",
>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>> *adev, unsigned long size,
>>>           }
>>>           return r;
>>>       }
>>> +
>>> +    bo->preferred_domains = initial_domain;
>>> +    bo->allowed_domains = initial_domain;
>>> +    if (type != ttm_bo_type_kernel &&
>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>> It's an ugly change back after bo creation! Do you think it's better 
>> than previous?
>
> Well it's better than moving the fallback handling into the core 
> functions and then adding a flag to disable it again because we don't 
> want it in the core function.
>
>>
>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>> directly pass preferred_domains and allowed_domains to amdgpu_bo_create.
>
> Then we would need three parameters, one where to create the BO now 
> and two for preferred/allowed domains.
>
> I think that in this case overwriting the placement from the initial 
> value looks cleaner to me.

Ping? Any more opinions on how to proceed?

I agree that neither solution is perfect, but updating the placement 
after we created the BO still sounds like the least painful to me.

Christian.

>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>> +
>>>       *obj = &bo->gem_base;
>>>         return 0;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 6d08cde8443c..854d18d5cff4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>> amdgpu_device *adev, unsigned long size,
>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>       INIT_LIST_HEAD(&bo->va);
>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>> -    bo->allowed_domains = bo->preferred_domains;
>>> -    if (type != ttm_bo_type_kernel &&
>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>> -
>>> +    bo->preferred_domains = domain;
>>> +    bo->allowed_domains = domain;
>>>       bo->flags = flags;
>>>     #ifdef CONFIG_X86_32
>>
>

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

Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[zhoucm1]

On 2018年04月16日 16:57, Christian König wrote:
> Am 11.04.2018 um 11:22 schrieb Christian König:
>> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>>
>>>
>>> On 2018年04月10日 21:42, Christian König wrote:
>>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>>> preferred domain to be untouched so that the BO has a cance to move 
>>>> back
>>>> to VRAM in the future.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 46b9ea4e6103..9dc0a190413c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>>> *adev, unsigned long size,
>>>>                    struct reservation_object *resv,
>>>>                    struct drm_gem_object **obj)
>>>>   {
>>>> +    uint32_t domain = initial_domain;
>>>>       struct amdgpu_bo *bo;
>>>>       int r;
>>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>       }
>>>>     retry:
>>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>>                    flags, type, resv, &bo);
>>>>       if (r) {
>>>>           if (r != -ERESTARTSYS) {
>>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>>> *adev, unsigned long size,
>>>>                   goto retry;
>>>>               }
>>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>                   goto retry;
>>>>               }
>>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, 
>>>> %u, %d)\n",
>>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>           }
>>>>           return r;
>>>>       }
>>>> +
>>>> +    bo->preferred_domains = initial_domain;
>>>> +    bo->allowed_domains = initial_domain;
>>>> +    if (type != ttm_bo_type_kernel &&
>>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>> It's an ugly change back after bo creation! Do you think it's better 
>>> than previous?
>>
>> Well it's better than moving the fallback handling into the core 
>> functions and then adding a flag to disable it again because we don't 
>> want it in the core function.
>>
>>>
>>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>>> directly pass preferred_domains and allowed_domains to 
>>> amdgpu_bo_create.
>>
>> Then we would need three parameters, one where to create the BO now 
>> and two for preferred/allowed domains.
>>
>> I think that in this case overwriting the placement from the initial 
>> value looks cleaner to me.
>
> Ping? Any more opinions on how to proceed?
No, I keep my opinion on this. Michel already gave your RB I remember.

Regards,
David Zhou
>
> I agree that neither solution is perfect, but updating the placement 
> after we created the BO still sounds like the least painful to me.
>
> Christian.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David Zhou
>>>> +
>>>>       *obj = &bo->gem_base;
>>>>         return 0;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index 6d08cde8443c..854d18d5cff4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>>       INIT_LIST_HEAD(&bo->va);
>>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>>> -    bo->allowed_domains = bo->preferred_domains;
>>>> -    if (type != ttm_bo_type_kernel &&
>>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>> -
>>>> +    bo->preferred_domains = domain;
>>>> +    bo->allowed_domains = domain;
>>>>       bo->flags = flags;
>>>>     #ifdef CONFIG_X86_32
>>>
>>
>

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

Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[zhoucm1]

On 2018年04月16日 17:04, zhoucm1 wrote:
>
>
> On 2018年04月16日 16:57, Christian König wrote:
>> Am 11.04.2018 um 11:22 schrieb Christian König:
>>> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>>>
>>>>
>>>> On 2018年04月10日 21:42, Christian König wrote:
>>>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>>>> preferred domain to be untouched so that the BO has a cance to 
>>>>> move back
>>>>> to VRAM in the future.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 46b9ea4e6103..9dc0a190413c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct 
>>>>> amdgpu_device *adev, unsigned long size,
>>>>>                    struct reservation_object *resv,
>>>>>                    struct drm_gem_object **obj)
>>>>>   {
>>>>> +    uint32_t domain = initial_domain;
>>>>>       struct amdgpu_bo *bo;
>>>>>       int r;
>>>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>>>> amdgpu_device *adev, unsigned long size,
>>>>>       }
>>>>>     retry:
>>>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>>>                    flags, type, resv, &bo);
>>>>>       if (r) {
>>>>>           if (r != -ERESTARTSYS) {
>>>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct 
>>>>> amdgpu_device *adev, unsigned long size,
>>>>>                   goto retry;
>>>>>               }
>>>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>                   goto retry;
>>>>>               }
>>>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, 
>>>>> %u, %d)\n",
>>>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct 
>>>>> amdgpu_device *adev, unsigned long size,
>>>>>           }
>>>>>           return r;
>>>>>       }
>>>>> +
>>>>> +    bo->preferred_domains = initial_domain;
>>>>> +    bo->allowed_domains = initial_domain;
>>>>> +    if (type != ttm_bo_type_kernel &&
>>>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>> It's an ugly change back after bo creation! Do you think it's 
>>>> better than previous?
>>>
>>> Well it's better than moving the fallback handling into the core 
>>> functions and then adding a flag to disable it again because we 
>>> don't want it in the core function.
>>>
>>>>
>>>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>>>> directly pass preferred_domains and allowed_domains to 
>>>> amdgpu_bo_create.
>>>
>>> Then we would need three parameters, one where to create the BO now 
>>> and two for preferred/allowed domains.
>>>
>>> I think that in this case overwriting the placement from the initial 
>>> value looks cleaner to me.
>>
>> Ping? Any more opinions on how to proceed?
> No, I keep my opinion on this. Michel already gave your RB I remember.
>
> Regards,
> David Zhou
>>
>> I agree that neither solution is perfect, but updating the placement 
>> after we created the BO still sounds like the least painful to me.
And I'm going to have patch for amdgpu_bo_create paramters, collect many 
paramters to one param struct like done in vm.

Regards,
David Zhou
>>
>> Christian.
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>> +
>>>>>       *obj = &bo->gem_base;
>>>>>         return 0;
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 6d08cde8443c..854d18d5cff4 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>>>> amdgpu_device *adev, unsigned long size,
>>>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>>>       INIT_LIST_HEAD(&bo->va);
>>>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>>>> -    bo->allowed_domains = bo->preferred_domains;
>>>>> -    if (type != ttm_bo_type_kernel &&
>>>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>> -
>>>>> +    bo->preferred_domains = domain;
>>>>> +    bo->allowed_domains = domain;
>>>>>       bo->flags = flags;
>>>>>     #ifdef CONFIG_X86_32
>>>>
>>>
>>
>
> _______________________________________________
> 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 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[Christian König]

Am 16.04.2018 um 11:21 schrieb zhoucm1:
>
>
> On 2018年04月16日 17:04, zhoucm1 wrote:
>>
>>
>> On 2018年04月16日 16:57, Christian König wrote:
>>> Am 11.04.2018 um 11:22 schrieb Christian König:
>>>> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>>>>
>>>>>
>>>>> On 2018年04月10日 21:42, Christian König wrote:
>>>>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>>>>> preferred domain to be untouched so that the BO has a cance to 
>>>>>> move back
>>>>>> to VRAM in the future.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index 46b9ea4e6103..9dc0a190413c 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct 
>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>                    struct reservation_object *resv,
>>>>>>                    struct drm_gem_object **obj)
>>>>>>   {
>>>>>> +    uint32_t domain = initial_domain;
>>>>>>       struct amdgpu_bo *bo;
>>>>>>       int r;
>>>>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>       }
>>>>>>     retry:
>>>>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>>>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>>>>                    flags, type, resv, &bo);
>>>>>>       if (r) {
>>>>>>           if (r != -ERESTARTSYS) {
>>>>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct 
>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>                   goto retry;
>>>>>>               }
>>>>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>                   goto retry;
>>>>>>               }
>>>>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, 
>>>>>> %u, %d)\n",
>>>>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct 
>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>           }
>>>>>>           return r;
>>>>>>       }
>>>>>> +
>>>>>> +    bo->preferred_domains = initial_domain;
>>>>>> +    bo->allowed_domains = initial_domain;
>>>>>> +    if (type != ttm_bo_type_kernel &&
>>>>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>> It's an ugly change back after bo creation! Do you think it's 
>>>>> better than previous?
>>>>
>>>> Well it's better than moving the fallback handling into the core 
>>>> functions and then adding a flag to disable it again because we 
>>>> don't want it in the core function.
>>>>
>>>>>
>>>>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>>>>> directly pass preferred_domains and allowed_domains to 
>>>>> amdgpu_bo_create.
>>>>
>>>> Then we would need three parameters, one where to create the BO now 
>>>> and two for preferred/allowed domains.
>>>>
>>>> I think that in this case overwriting the placement from the 
>>>> initial value looks cleaner to me.
>>>
>>> Ping? Any more opinions on how to proceed?
>> No, I keep my opinion on this. Michel already gave your RB I remember.
>>
>> Regards,
>> David Zhou
>>>
>>> I agree that neither solution is perfect, but updating the placement 
>>> after we created the BO still sounds like the least painful to me.
> And I'm going to have patch for amdgpu_bo_create paramters, collect 
> many paramters to one param struct like done in vm.

Oh, good idea! That would be a good solution as well I think.

Then we can provide amdgpu_bo_do_create() with both the preferred and 
the allowed domain.

Do you want to keep working on this or should I?

Thanks,
Christian.

>
> Regards,
> David Zhou
>>>
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>> +
>>>>>>       *obj = &bo->gem_base;
>>>>>>         return 0;
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 6d08cde8443c..854d18d5cff4 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>>>>       INIT_LIST_HEAD(&bo->va);
>>>>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>>>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>>>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>>>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>>>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>>>>> -    bo->allowed_domains = bo->preferred_domains;
>>>>>> -    if (type != ttm_bo_type_kernel &&
>>>>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>> -
>>>>>> +    bo->preferred_domains = domain;
>>>>>> +    bo->allowed_domains = domain;
>>>>>>       bo->flags = flags;
>>>>>>     #ifdef CONFIG_X86_32
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> 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 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[zhoucm1]

On 2018年04月16日 17:36, Christian König wrote:
> Am 16.04.2018 um 11:21 schrieb zhoucm1:
>>
>>
>> On 2018年04月16日 17:04, zhoucm1 wrote:
>>>
>>>
>>> On 2018年04月16日 16:57, Christian König wrote:
>>>> Am 11.04.2018 um 11:22 schrieb Christian König:
>>>>> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>>>>>
>>>>>>
>>>>>> On 2018年04月10日 21:42, Christian König wrote:
>>>>>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>>>>>> preferred domain to be untouched so that the BO has a cance to 
>>>>>>> move back
>>>>>>> to VRAM in the future.
>>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>>>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> index 46b9ea4e6103..9dc0a190413c 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct 
>>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>>                    struct reservation_object *resv,
>>>>>>>                    struct drm_gem_object **obj)
>>>>>>>   {
>>>>>>> +    uint32_t domain = initial_domain;
>>>>>>>       struct amdgpu_bo *bo;
>>>>>>>       int r;
>>>>>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>>       }
>>>>>>>     retry:
>>>>>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>>>>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>>>>>                    flags, type, resv, &bo);
>>>>>>>       if (r) {
>>>>>>>           if (r != -ERESTARTSYS) {
>>>>>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct 
>>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>>                   goto retry;
>>>>>>>               }
>>>>>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>>>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>                   goto retry;
>>>>>>>               }
>>>>>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, 
>>>>>>> %u, %d)\n",
>>>>>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct 
>>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>>           }
>>>>>>>           return r;
>>>>>>>       }
>>>>>>> +
>>>>>>> +    bo->preferred_domains = initial_domain;
>>>>>>> +    bo->allowed_domains = initial_domain;
>>>>>>> +    if (type != ttm_bo_type_kernel &&
>>>>>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>> It's an ugly change back after bo creation! Do you think it's 
>>>>>> better than previous?
>>>>>
>>>>> Well it's better than moving the fallback handling into the core 
>>>>> functions and then adding a flag to disable it again because we 
>>>>> don't want it in the core function.
>>>>>
>>>>>>
>>>>>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>>>>>> directly pass preferred_domains and allowed_domains to 
>>>>>> amdgpu_bo_create.
>>>>>
>>>>> Then we would need three parameters, one where to create the BO 
>>>>> now and two for preferred/allowed domains.
>>>>>
>>>>> I think that in this case overwriting the placement from the 
>>>>> initial value looks cleaner to me.
>>>>
>>>> Ping? Any more opinions on how to proceed?
>>> No, I keep my opinion on this. Michel already gave your RB I remember.
>>>
>>> Regards,
>>> David Zhou
>>>>
>>>> I agree that neither solution is perfect, but updating the 
>>>> placement after we created the BO still sounds like the least 
>>>> painful to me.
>> And I'm going to have patch for amdgpu_bo_create paramters, collect 
>> many paramters to one param struct like done in vm.
>
> Oh, good idea! That would be a good solution as well I think.
>
> Then we can provide amdgpu_bo_do_create() with both the preferred and 
> the allowed domain.
>
> Do you want to keep working on this or should I?
you can push your reverting patch#1 and #2 first. we can re-do this 
patch#3 again after I pass my patches of collecting parameters for 
amdgpu_bo_create.

Regards,
David Zhou
>
> Thanks,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> David Zhou
>>>>>>> +
>>>>>>>       *obj = &bo->gem_base;
>>>>>>>         return 0;
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> index 6d08cde8443c..854d18d5cff4 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>>>>>> amdgpu_device *adev, unsigned long size,
>>>>>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>>>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>>>>>       INIT_LIST_HEAD(&bo->va);
>>>>>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>>>>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>>>>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>>>>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>>>>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>>>>>> -    bo->allowed_domains = bo->preferred_domains;
>>>>>>> -    if (type != ttm_bo_type_kernel &&
>>>>>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>> -
>>>>>>> +    bo->preferred_domains = domain;
>>>>>>> +    bo->allowed_domains = domain;
>>>>>>>       bo->flags = flags;
>>>>>>>     #ifdef CONFIG_X86_32
>>>>>>
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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