Re: [PATCH v3 6/6] drm/ttm: Switch to using the new res callback

2022-07-29 Thread Arunpravin Paneer Selvam 




On 7/28/2022 9:07 PM, Matthew Auld wrote:

On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:

Apply new intersect and compatible callback instead
of having a generic placement range verfications.

v2: Added a separate callback for compatiblilty
 checks (Christian)

Signed-off-by: Christian König 
Signed-off-by: Arunpravin Paneer Selvam 



There is also some code at the bottom of i915_ttm_buddy_man_alloc() 
playing games with res->start, which I think can be safely deleted 
with this series (now that we have proper ->compatible() hook).


Also, is the plan to remove res->start completely, or does that still 
have a use?
yes we should remove res->start completely, I am working on it, planning 
to send in a separate series as amdgpu uses it in many places, and in 
some places we set res->start to AMDGPU_BO_INVALID_OFFSET,
I should find an alternative to indicate the invalid offset BO. Also, 
res->start used in drm/drm_gem_vram_helper.c at drm_gem_vram_pg_offset() 
function. I am removing all the dependencies, I will send the
patches in a separate series. I think i915 doesn't use res->start in its 
own driver code.



---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++--
  drivers/gpu/drm/ttm/ttm_bo.c    |  9 +++--
  drivers/gpu/drm/ttm/ttm_resource.c  |  5 +--
  3 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

index 170935c294f5..7d25a10395c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct 
amdgpu_device *adev, struct ttm_tt *ttm,
  static bool amdgpu_ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,

  const struct ttm_place *place)
  {
-    unsigned long num_pages = bo->resource->num_pages;
  struct dma_resv_iter resv_cursor;
-    struct amdgpu_res_cursor cursor;
  struct dma_fence *f;
  +    if (!amdgpu_bo_is_amdgpu_bo(bo))
+    return ttm_bo_eviction_valuable(bo, place);
+
  /* Swapout? */
  if (bo->resource->mem_type == TTM_PL_SYSTEM)
  return true;
@@ -1351,40 +1352,20 @@ static bool 
amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,

  return false;
  }
  -    switch (bo->resource->mem_type) {
-    case AMDGPU_PL_PREEMPT:
-    /* Preemptible BOs don't own system resources managed by the
- * driver (pages, VRAM, GART space). They point to resources
- * owned by someone else (e.g. pageable memory in user mode
- * or a DMABuf). They are used in a preemptible context so we
- * can guarantee no deadlocks and good QoS in case of MMU
- * notifiers or DMABuf move notifiers from the resource owner.
- */
+    /* Preemptible BOs don't own system resources managed by the
+ * driver (pages, VRAM, GART space). They point to resources
+ * owned by someone else (e.g. pageable memory in user mode
+ * or a DMABuf). They are used in a preemptible context so we
+ * can guarantee no deadlocks and good QoS in case of MMU
+ * notifiers or DMABuf move notifiers from the resource owner.
+ */
+    if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
  return false;
-    case TTM_PL_TT:
-    if (amdgpu_bo_is_amdgpu_bo(bo) &&
-    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
-    return false;
-    return true;
  -    case TTM_PL_VRAM:
-    /* Check each drm MM node individually */
-    amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
- );
-    while (cursor.remaining) {
-    if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
-    && !(place->lpfn &&
- place->lpfn <= PFN_DOWN(cursor.start)))
-    return true;
-
-    amdgpu_res_next(, cursor.size);
-    }
+    if (bo->resource->mem_type == TTM_PL_TT &&
+    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
  return false;
  -    default:
-    break;
-    }
-
  return ttm_bo_eviction_valuable(bo, place);
  }
  diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
b/drivers/gpu/drm/ttm/ttm_bo.c

index c1bd006a5525..03409409e43e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object 
*bo,

  bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
    const struct ttm_place *place)
  {
+    struct ttm_resource *res = bo->resource;
+    struct ttm_device *bdev = bo->bdev;
+
  dma_resv_assert_held(bo->base.resv);
  if (bo->resource->mem_type == TTM_PL_SYSTEM)
  return true;
@@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,

  /* Don't evict this BO if it's outside of the
   * requested placement range
   */
-    if (place->fpfn >= (bo->resource->start +

Re: [PATCH v3 6/6] drm/ttm: Switch to using the new res callback

2022-07-28 Thread Matthew Auld 

On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:

Apply new intersect and compatible callback instead
of having a generic placement range verfications.

v2: Added a separate callback for compatiblilty
 checks (Christian)

Signed-off-by: Christian König 
Signed-off-by: Arunpravin Paneer Selvam 


There is also some code at the bottom of i915_ttm_buddy_man_alloc() 
playing games with res->start, which I think can be safely deleted with 
this series (now that we have proper ->compatible() hook).


Also, is the plan to remove res->start completely, or does that still 
have a use?



---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++--
  drivers/gpu/drm/ttm/ttm_bo.c|  9 +++--
  drivers/gpu/drm/ttm/ttm_resource.c  |  5 +--
  3 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 170935c294f5..7d25a10395c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device 
*adev, struct ttm_tt *ttm,
  static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
const struct ttm_place *place)
  {
-   unsigned long num_pages = bo->resource->num_pages;
struct dma_resv_iter resv_cursor;
-   struct amdgpu_res_cursor cursor;
struct dma_fence *f;
  
+	if (!amdgpu_bo_is_amdgpu_bo(bo))

+   return ttm_bo_eviction_valuable(bo, place);
+
/* Swapout? */
if (bo->resource->mem_type == TTM_PL_SYSTEM)
return true;
@@ -1351,40 +1352,20 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,
return false;
}
  
-	switch (bo->resource->mem_type) {

-   case AMDGPU_PL_PREEMPT:
-   /* Preemptible BOs don't own system resources managed by the
-* driver (pages, VRAM, GART space). They point to resources
-* owned by someone else (e.g. pageable memory in user mode
-* or a DMABuf). They are used in a preemptible context so we
-* can guarantee no deadlocks and good QoS in case of MMU
-* notifiers or DMABuf move notifiers from the resource owner.
-*/
+   /* Preemptible BOs don't own system resources managed by the
+* driver (pages, VRAM, GART space). They point to resources
+* owned by someone else (e.g. pageable memory in user mode
+* or a DMABuf). They are used in a preemptible context so we
+* can guarantee no deadlocks and good QoS in case of MMU
+* notifiers or DMABuf move notifiers from the resource owner.
+*/
+   if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
return false;
-   case TTM_PL_TT:
-   if (amdgpu_bo_is_amdgpu_bo(bo) &&
-   amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
-   return false;
-   return true;
  
-	case TTM_PL_VRAM:

-   /* Check each drm MM node individually */
-   amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
-);
-   while (cursor.remaining) {
-   if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
-   && !(place->lpfn &&
-place->lpfn <= PFN_DOWN(cursor.start)))
-   return true;
-
-   amdgpu_res_next(, cursor.size);
-   }
+   if (bo->resource->mem_type == TTM_PL_TT &&
+   amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
return false;
  
-	default:

-   break;
-   }
-
return ttm_bo_eviction_valuable(bo, place);
  }
  
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c

index c1bd006a5525..03409409e43e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
  bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
  const struct ttm_place *place)
  {
+   struct ttm_resource *res = bo->resource;
+   struct ttm_device *bdev = bo->bdev;
+
dma_resv_assert_held(bo->base.resv);
if (bo->resource->mem_type == TTM_PL_SYSTEM)
return true;
@@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
/* Don't evict this BO if it's outside of the
 * requested placement range
 */
-   if (place->fpfn >= (bo->resource->start + bo->resource->num_pages) ||
-   (place->lpfn && place->lpfn <= bo->resource->start))
-   return false;
-
-   return true;
+   return ttm_resource_intersect(bdev, res, place, bo->base.size);
  }

[PATCH v3 6/6] drm/ttm: Switch to using the new res callback

2022-07-28 Thread Arunpravin Paneer Selvam 
Apply new intersect and compatible callback instead
of having a generic placement range verfications.

v2: Added a separate callback for compatiblilty
checks (Christian)

Signed-off-by: Christian König 
Signed-off-by: Arunpravin Paneer Selvam 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++--
 drivers/gpu/drm/ttm/ttm_bo.c|  9 +++--
 drivers/gpu/drm/ttm/ttm_resource.c  |  5 +--
 3 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 170935c294f5..7d25a10395c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device 
*adev, struct ttm_tt *ttm,
 static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
const struct ttm_place *place)
 {
-   unsigned long num_pages = bo->resource->num_pages;
struct dma_resv_iter resv_cursor;
-   struct amdgpu_res_cursor cursor;
struct dma_fence *f;
 
+   if (!amdgpu_bo_is_amdgpu_bo(bo))
+   return ttm_bo_eviction_valuable(bo, place);
+
/* Swapout? */
if (bo->resource->mem_type == TTM_PL_SYSTEM)
return true;
@@ -1351,40 +1352,20 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,
return false;
}
 
-   switch (bo->resource->mem_type) {
-   case AMDGPU_PL_PREEMPT:
-   /* Preemptible BOs don't own system resources managed by the
-* driver (pages, VRAM, GART space). They point to resources
-* owned by someone else (e.g. pageable memory in user mode
-* or a DMABuf). They are used in a preemptible context so we
-* can guarantee no deadlocks and good QoS in case of MMU
-* notifiers or DMABuf move notifiers from the resource owner.
-*/
+   /* Preemptible BOs don't own system resources managed by the
+* driver (pages, VRAM, GART space). They point to resources
+* owned by someone else (e.g. pageable memory in user mode
+* or a DMABuf). They are used in a preemptible context so we
+* can guarantee no deadlocks and good QoS in case of MMU
+* notifiers or DMABuf move notifiers from the resource owner.
+*/
+   if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
return false;
-   case TTM_PL_TT:
-   if (amdgpu_bo_is_amdgpu_bo(bo) &&
-   amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
-   return false;
-   return true;
 
-   case TTM_PL_VRAM:
-   /* Check each drm MM node individually */
-   amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
-);
-   while (cursor.remaining) {
-   if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
-   && !(place->lpfn &&
-place->lpfn <= PFN_DOWN(cursor.start)))
-   return true;
-
-   amdgpu_res_next(, cursor.size);
-   }
+   if (bo->resource->mem_type == TTM_PL_TT &&
+   amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
return false;
 
-   default:
-   break;
-   }
-
return ttm_bo_eviction_valuable(bo, place);
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c1bd006a5525..03409409e43e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
 bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
  const struct ttm_place *place)
 {
+   struct ttm_resource *res = bo->resource;
+   struct ttm_device *bdev = bo->bdev;
+
dma_resv_assert_held(bo->base.resv);
if (bo->resource->mem_type == TTM_PL_SYSTEM)
return true;
@@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
/* Don't evict this BO if it's outside of the
 * requested placement range
 */
-   if (place->fpfn >= (bo->resource->start + bo->resource->num_pages) ||
-   (place->lpfn && place->lpfn <= bo->resource->start))
-   return false;
-
-   return true;
+   return ttm_resource_intersect(bdev, res, place, bo->base.size);
 }
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c 
b/drivers/gpu/drm/ttm/ttm_resource.c
index 84a6fe9e976e..c745faf72b1a 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct ttm_resource 
*res,