Re: [PATCH v6 1/6] drm/ttm: Add new callbacks to ttm res mgr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 06.09.22 um 21:58 schrieb Daniel Vetter:
On Tue, Aug 16, 2022 at 10:33:16AM +0530, Arunpravin Paneer Selvam wrote:

On 8/15/2022 4:35 PM, Christian König wrote:
Am 12.08.22 um 15:30 schrieb Arunpravin Paneer Selvam:
We are adding two new callbacks to ttm resource manager
function to handle intersection and compatibility of
placement and resources.

v2: move the amdgpu and ttm_range_manager changes to
      separate patches (Christian)
v3: rename "intersect" to "intersects" (Matthew)
v4: move !place check to the !res if and return false
      in ttm_resource_compatible() function (Christian)
v5: move bits of code from patch number 6 to avoid
      temporary driver breakup (Christian)

Signed-off-by: Christian König <christian.koenig@xxxxxxx>
Signed-off-by: Arunpravin Paneer Selvam
<Arunpravin.PaneerSelvam@xxxxxxx>
Patch #6 could still be cleaned up more now that we have the workaround
code in patch #1, but that not really a must have.

Reviewed-by: Christian König <christian.koenig@xxxxxxx> for the entire
series.

Do you already have commit rights?
Hi Christian,
I applied for drm-misc commit rights, waiting for the project maintainers to
approve my request.
Why do all drivers have to implement the current behaviour? Can we have a
default implementation, which gets called if nothing is set instead?

We do have a default implementation in the range manager which is used by radeon, GEM VRAM helpers, VMWGFX and amdgpu (but there only for some domains).

I'm a bit confused why the bloat here ...

Drivers do have specialized implementations of the backend, e.g. VMWGFX have his handle backend, amdgpu the VRAM backend with special placements, i915 is completely special as well.

Here we only move the decision if resources intersect or are compatible into those specialized backends. Previously we had all this in a centralized callback for all backends of a driver.

See the switch in amdgpu_ttm_bo_eviction_valuable() for an example. Final goal is to move all this stuff into the specialized backends and remove this callback.

The only driver where I couldn't figure out why we have duplicated all this from the standard implementation is Nouveau.

Also please document new callbacks precisely with inline kerneldoc. I know
ttm docs aren't great yet, but they don't get better if we don't start
somewhere. I think the in-depth comments for modeset vfuncs (e.g. in
drm_modeset_helper_vtables.h) are a good standard here.

I thought we already did that. Please be a bit more specific.

Thanks,
Christian.

-Daniel

Thanks,
Arun
Regards,
Christian.

---
   drivers/gpu/drm/ttm/ttm_bo.c       |  9 ++--
   drivers/gpu/drm/ttm/ttm_resource.c | 77 +++++++++++++++++++++++++++++-
   include/drm/ttm/ttm_resource.h     | 40 ++++++++++++++++
   3 files changed, 119 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c1bd006a5525..f066e8124c50 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_intersects(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 20f9adcc3235..0d1f862a582b 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -253,10 +253,84 @@ void ttm_resource_free(struct
ttm_buffer_object *bo, struct ttm_resource **res)
   }
   EXPORT_SYMBOL(ttm_resource_free);
   +/**
+ * ttm_resource_intersects - test for intersection
+ *
+ * @bdev: TTM device structure
+ * @res: The resource to test
+ * @place: The placement to test
+ * @size: How many bytes the new allocation needs.
+ *
+ * Test if @res intersects with @place and @size. Used for testing
if evictions
+ * are valueable or not.
+ *
+ * Returns true if the res placement intersects with @place and @size.
+ */
+bool ttm_resource_intersects(struct ttm_device *bdev,
+                 struct ttm_resource *res,
+                 const struct ttm_place *place,
+                 size_t size)
+{
+    struct ttm_resource_manager *man;
+
+    if (!res)
+        return false;
+
+    if (!place)
+        return true;
+
+    man = ttm_manager_type(bdev, res->mem_type);
+    if (!man->func->intersects) {
+        if (place->fpfn >= (res->start + res->num_pages) ||
+            (place->lpfn && place->lpfn <= res->start))
+            return false;
+
+        return true;
+    }
+
+    return man->func->intersects(man, res, place, size);
+}
+
+/**
+ * ttm_resource_compatible - test for compatibility
+ *
+ * @bdev: TTM device structure
+ * @res: The resource to test
+ * @place: The placement to test
+ * @size: How many bytes the new allocation needs.
+ *
+ * Test if @res compatible with @place and @size.
+ *
+ * Returns true if the res placement compatible with @place and @size.
+ */
+bool ttm_resource_compatible(struct ttm_device *bdev,
+                 struct ttm_resource *res,
+                 const struct ttm_place *place,
+                 size_t size)
+{
+    struct ttm_resource_manager *man;
+
+    if (!res || !place)
+        return false;
+
+    man = ttm_manager_type(bdev, res->mem_type);
+    if (!man->func->compatible) {
+        if (res->start < place->fpfn ||
+            (place->lpfn && (res->start + res->num_pages) >
place->lpfn))
+            return false;
+
+        return true;
+    }
+
+    return man->func->compatible(man, res, place, size);
+}
+
   static bool ttm_resource_places_compat(struct ttm_resource *res,
                          const struct ttm_place *places,
                          unsigned num_placement)
   {
+    struct ttm_buffer_object *bo = res->bo;
+    struct ttm_device *bdev = bo->bdev;
       unsigned i;
         if (res->placement & TTM_PL_FLAG_TEMPORARY)
@@ -265,8 +339,7 @@ static bool ttm_resource_places_compat(struct
ttm_resource *res,
       for (i = 0; i < num_placement; i++) {
           const struct ttm_place *heap = &places[i];
   -        if (res->start < heap->fpfn || (heap->lpfn &&
-            (res->start + res->num_pages) > heap->lpfn))
+        if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
               continue;
             if ((res->mem_type == heap->mem_type) &&
diff --git a/include/drm/ttm/ttm_resource.h
b/include/drm/ttm/ttm_resource.h
index ca89a48c2460..5afc6d664fde 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -88,6 +88,38 @@ struct ttm_resource_manager_func {
       void (*free)(struct ttm_resource_manager *man,
                struct ttm_resource *res);
   +    /**
+     * struct ttm_resource_manager_func member intersects
+     *
+     * @man: Pointer to a memory type manager.
+     * @res: Pointer to a struct ttm_resource to be checked.
+     * @place: Placement to check against.
+     * @size: Size of the check.
+     *
+     * Test if @res intersects with @place + @size. Used to judge if
+     * evictions are valueable or not.
+     */
+    bool (*intersects)(struct ttm_resource_manager *man,
+               struct ttm_resource *res,
+               const struct ttm_place *place,
+               size_t size);
+
+    /**
+     * struct ttm_resource_manager_func member compatible
+     *
+     * @man: Pointer to a memory type manager.
+     * @res: Pointer to a struct ttm_resource to be checked.
+     * @place: Placement to check against.
+     * @size: Size of the check.
+     *
+     * Test if @res compatible with @place + @size. Used to check of
+     * the need to move the backing store or not.
+     */
+    bool (*compatible)(struct ttm_resource_manager *man,
+               struct ttm_resource *res,
+               const struct ttm_place *place,
+               size_t size);
+
       /**
        * struct ttm_resource_manager_func member debug
        *
@@ -329,6 +361,14 @@ int ttm_resource_alloc(struct ttm_buffer_object
*bo,
                  const struct ttm_place *place,
                  struct ttm_resource **res);
   void ttm_resource_free(struct ttm_buffer_object *bo, struct
ttm_resource **res);
+bool ttm_resource_intersects(struct ttm_device *bdev,
+                 struct ttm_resource *res,
+                 const struct ttm_place *place,
+                 size_t size);
+bool ttm_resource_compatible(struct ttm_device *bdev,
+                 struct ttm_resource *res,
+                 const struct ttm_place *place,
+                 size_t size);
   bool ttm_resource_compat(struct ttm_resource *res,
                struct ttm_placement *placement);
   void ttm_resource_set_bo(struct ttm_resource *res,

base-commit: 730c2bf4ad395acf0aa0820535fdb8ea6abe5df1




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux