* [PATCH 1/3] drm/amdgpu: revert "add new bo flag that indicates BOs don't need fallback (v2)"
@ 2018-04-10 13:42 Christian König
[not found] ` <20180410134240.9515-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2018-04-10 13:42 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/amdgpu: revert "Don't change preferred domian when fallback GTT v6"
[not found] ` <20180410134240.9515-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-10 13:42 ` Christian König
2018-04-10 13:42 ` [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling Christian König
1 sibling, 0 replies; 10+ messages in thread
From: Christian König @ 2018-04-10 13:42 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <20180410134240.9515-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2018-04-10 13:42 ` [PATCH 2/3] drm/amdgpu: revert "Don't change preferred domian when fallback GTT v6" Christian König
@ 2018-04-10 13:42 ` Christian König
[not found] ` <20180410134240.9515-3-christian.koenig-5C7GfCeVMHo@public.gmane.org>
1 sibling, 1 reply; 10+ messages in thread
From: Christian König @ 2018-04-10 13:42 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <20180410134240.9515-3-christian.koenig-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-11 2:38 ` zhoucm1
[not found] ` <06295e11-12f1-6ca9-133e-f906cdf04ed7-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: zhoucm1 @ 2018-04-11 2:38 UTC (permalink / raw)
To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <06295e11-12f1-6ca9-133e-f906cdf04ed7-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-11 9:22 ` Christian König
[not found] ` <29b09ef5-a88a-daff-fc13-c875f975e26e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2018-04-11 9:22 UTC (permalink / raw)
To: zhoucm1, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <29b09ef5-a88a-daff-fc13-c875f975e26e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2018-04-16 8:57 ` Christian König
[not found] ` <47328a0b-e56b-24fb-0838-271a5127a3bc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2018-04-16 8:57 UTC (permalink / raw)
To: zhoucm1, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <47328a0b-e56b-24fb-0838-271a5127a3bc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2018-04-16 9:04 ` zhoucm1
[not found] ` <845e35b3-3b2c-0ed0-ba27-9eb4e603df21-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: zhoucm1 @ 2018-04-16 9:04 UTC (permalink / raw)
To: christian.koenig-5C7GfCeVMHo, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <845e35b3-3b2c-0ed0-ba27-9eb4e603df21-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-16 9:21 ` zhoucm1
[not found] ` <0df2d99a-a997-3842-e063-18543cd49aa2-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: zhoucm1 @ 2018-04-16 9:21 UTC (permalink / raw)
To: christian.koenig-5C7GfCeVMHo, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <0df2d99a-a997-3842-e063-18543cd49aa2-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-16 9:36 ` Christian König
[not found] ` <f961f48e-7b37-d988-144c-5f6cfaaa9ec0-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2018-04-16 9:36 UTC (permalink / raw)
To: zhoucm1, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling
[not found] ` <f961f48e-7b37-d988-144c-5f6cfaaa9ec0-5C7GfCeVMHo@public.gmane.org>
@ 2018-04-16 10:38 ` zhoucm1
0 siblings, 0 replies; 10+ messages in thread
From: zhoucm1 @ 2018-04-16 10:38 UTC (permalink / raw)
To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-04-16 10:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-10 13:42 [PATCH 1/3] drm/amdgpu: revert "add new bo flag that indicates BOs don't need fallback (v2)" Christian König
[not found] ` <20180410134240.9515-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2018-04-10 13:42 ` [PATCH 2/3] drm/amdgpu: revert "Don't change preferred domian when fallback GTT v6" Christian König
2018-04-10 13:42 ` [PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling Christian König
[not found] ` <20180410134240.9515-3-christian.koenig-5C7GfCeVMHo@public.gmane.org>
2018-04-11 2:38 ` zhoucm1
[not found] ` <06295e11-12f1-6ca9-133e-f906cdf04ed7-5C7GfCeVMHo@public.gmane.org>
2018-04-11 9:22 ` Christian König
[not found] ` <29b09ef5-a88a-daff-fc13-c875f975e26e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-04-16 8:57 ` Christian König
[not found] ` <47328a0b-e56b-24fb-0838-271a5127a3bc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-04-16 9:04 ` zhoucm1
[not found] ` <845e35b3-3b2c-0ed0-ba27-9eb4e603df21-5C7GfCeVMHo@public.gmane.org>
2018-04-16 9:21 ` zhoucm1
[not found] ` <0df2d99a-a997-3842-e063-18543cd49aa2-5C7GfCeVMHo@public.gmane.org>
2018-04-16 9:36 ` Christian König
[not found] ` <f961f48e-7b37-d988-144c-5f6cfaaa9ec0-5C7GfCeVMHo@public.gmane.org>
2018-04-16 10:38 ` zhoucm1
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.