Re: [Intel-gfx] [CI v4 12/13] drm/i915/ttm: disallow CPU fallback mode for ccs pages
On 25/07/2022 16:09, Matthew Auld wrote:
Hi,
On 25/07/2022 15:55, Tvrtko Ursulin wrote:
Hi Matt,
On 29/06/2022 18:43, Matthew Auld wrote:
Falling back to memcpy/memset shouldn't be allowed if we know we have
CCS state to manage using the blitter. Otherwise we are potentially
leaving the aux CCS state in an unknown state, which smells like an info
leak.
Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for
Flat-ccs objects")
This is marking the patch for 5.19-rc, but it not apply since the code
seems a bit different. There is no i915_ttm_memcpy_allowed to start
with, which only comes in bfe53be268af ("drm/i915/ttm: handle blitter
failure on DG2"), which is for 5.20.
Do you think a version of this patch for 5.19 is needed and if so
could you, or someone in the know, cook one up today or tomorrow at
the latest?
It needs almost everything in bfe53be268af to close all the holes,
AFAIK. But then again this is only for DG2, which is still hidden behind
the force_probe stuff (I think), so perhaps not strictly needed for
5.19? What do you think?
It is under force probe. Good point - I agree we then do not have to be
concerned by it. Thanks!
Regards,
Tvrtko
Regards,
Tvrtko
Signed-off-by: Matthew Auld
Cc: Thomas Hellström
Cc: Lionel Landwerlin
Cc: Tvrtko Ursulin
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: Akeem G Abodunrin
Cc: Ramalingam C
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 26
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 18 --
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 +++
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 642a5d59ce26..ccec4055fde3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -717,6 +717,32 @@ bool i915_gem_object_placement_possible(struct
drm_i915_gem_object *obj,
return false;
}
+/**
+ * i915_gem_object_needs_ccs_pages - Check whether the object
requires extra
+ * pages when placed in system-memory, in order to save and later
restore the
+ * flat-CCS aux state when the object is moved between local-memory and
+ * system-memory
+ * @obj: Pointer to the object
+ *
+ * Return: True if the object needs extra ccs pages. False otherwise.
+ */
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
+{
+ bool lmem_placement = false;
+ int i;
+
+ for (i = 0; i < obj->mm.n_placements; i++) {
+ /* Compression is not allowed for the objects with smem
placement */
+ if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
+ return false;
+ if (!lmem_placement &&
+ obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
+ lmem_placement = true;
+ }
+
+ return lmem_placement;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_DELAYED_WORK(>mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 0bf3ee27a2a8..6f0a3ce35567 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -618,6 +618,8 @@ int i915_gem_object_wait_migration(struct
drm_i915_gem_object *obj,
bool i915_gem_object_placement_possible(struct drm_i915_gem_object
*obj,
enum intel_memory_type type);
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj);
+
int shmem_sg_alloc_table(struct drm_i915_private *i915, struct
sg_table *st,
size_t size, struct intel_memory_region *mr,
struct address_space *mapping,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 098409a33e10..7e1f8b83077f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -266,24 +266,6 @@ static const struct i915_refct_sgt_ops
tt_rsgt_ops = {
.release = i915_ttm_tt_release
};
-static inline bool
-i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
-{
- bool lmem_placement = false;
- int i;
-
- for (i = 0; i < obj->mm.n_placements; i++) {
- /* Compression is not allowed for the objects with smem
placement */
- if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
- return false;
- if (!lmem_placement &&
- obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
- lmem_placement = true;
- }
-
- return lmem_placement;
-}
-
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index df14ac81c128..9a7e50534b84 100644
Re: [Intel-gfx] [CI v4 12/13] drm/i915/ttm: disallow CPU fallback mode for ccs pages
Hi,
On 25/07/2022 15:55, Tvrtko Ursulin wrote:
Hi Matt,
On 29/06/2022 18:43, Matthew Auld wrote:
Falling back to memcpy/memset shouldn't be allowed if we know we have
CCS state to manage using the blitter. Otherwise we are potentially
leaving the aux CCS state in an unknown state, which smells like an info
leak.
Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for
Flat-ccs objects")
This is marking the patch for 5.19-rc, but it not apply since the code
seems a bit different. There is no i915_ttm_memcpy_allowed to start
with, which only comes in bfe53be268af ("drm/i915/ttm: handle blitter
failure on DG2"), which is for 5.20.
Do you think a version of this patch for 5.19 is needed and if so could
you, or someone in the know, cook one up today or tomorrow at the latest?
It needs almost everything in bfe53be268af to close all the holes,
AFAIK. But then again this is only for DG2, which is still hidden behind
the force_probe stuff (I think), so perhaps not strictly needed for
5.19? What do you think?
Regards,
Tvrtko
Signed-off-by: Matthew Auld
Cc: Thomas Hellström
Cc: Lionel Landwerlin
Cc: Tvrtko Ursulin
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: Akeem G Abodunrin
Cc: Ramalingam C
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 26
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 18 --
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 +++
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 642a5d59ce26..ccec4055fde3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -717,6 +717,32 @@ bool i915_gem_object_placement_possible(struct
drm_i915_gem_object *obj,
return false;
}
+/**
+ * i915_gem_object_needs_ccs_pages - Check whether the object
requires extra
+ * pages when placed in system-memory, in order to save and later
restore the
+ * flat-CCS aux state when the object is moved between local-memory and
+ * system-memory
+ * @obj: Pointer to the object
+ *
+ * Return: True if the object needs extra ccs pages. False otherwise.
+ */
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
+{
+ bool lmem_placement = false;
+ int i;
+
+ for (i = 0; i < obj->mm.n_placements; i++) {
+ /* Compression is not allowed for the objects with smem
placement */
+ if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
+ return false;
+ if (!lmem_placement &&
+ obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
+ lmem_placement = true;
+ }
+
+ return lmem_placement;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_DELAYED_WORK(>mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 0bf3ee27a2a8..6f0a3ce35567 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -618,6 +618,8 @@ int i915_gem_object_wait_migration(struct
drm_i915_gem_object *obj,
bool i915_gem_object_placement_possible(struct drm_i915_gem_object
*obj,
enum intel_memory_type type);
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj);
+
int shmem_sg_alloc_table(struct drm_i915_private *i915, struct
sg_table *st,
size_t size, struct intel_memory_region *mr,
struct address_space *mapping,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 098409a33e10..7e1f8b83077f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -266,24 +266,6 @@ static const struct i915_refct_sgt_ops
tt_rsgt_ops = {
.release = i915_ttm_tt_release
};
-static inline bool
-i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
-{
- bool lmem_placement = false;
- int i;
-
- for (i = 0; i < obj->mm.n_placements; i++) {
- /* Compression is not allowed for the objects with smem
placement */
- if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
- return false;
- if (!lmem_placement &&
- obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
- lmem_placement = true;
- }
-
- return lmem_placement;
-}
-
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index df14ac81c128..9a7e50534b84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -435,6 +435,9 @@ i915_ttm_memcpy_work_arm(struct
Re: [Intel-gfx] [CI v4 12/13] drm/i915/ttm: disallow CPU fallback mode for ccs pages
Hi Matt,
On 29/06/2022 18:43, Matthew Auld wrote:
Falling back to memcpy/memset shouldn't be allowed if we know we have
CCS state to manage using the blitter. Otherwise we are potentially
leaving the aux CCS state in an unknown state, which smells like an info
leak.
Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs
objects")
This is marking the patch for 5.19-rc, but it not apply since the code
seems a bit different. There is no i915_ttm_memcpy_allowed to start
with, which only comes in bfe53be268af ("drm/i915/ttm: handle blitter
failure on DG2"), which is for 5.20.
Do you think a version of this patch for 5.19 is needed and if so could
you, or someone in the know, cook one up today or tomorrow at the latest?
Regards,
Tvrtko
Signed-off-by: Matthew Auld
Cc: Thomas Hellström
Cc: Lionel Landwerlin
Cc: Tvrtko Ursulin
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: Akeem G Abodunrin
Cc: Ramalingam C
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 26
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 18 --
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 +++
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 642a5d59ce26..ccec4055fde3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -717,6 +717,32 @@ bool i915_gem_object_placement_possible(struct
drm_i915_gem_object *obj,
return false;
}
+/**
+ * i915_gem_object_needs_ccs_pages - Check whether the object requires extra
+ * pages when placed in system-memory, in order to save and later restore the
+ * flat-CCS aux state when the object is moved between local-memory and
+ * system-memory
+ * @obj: Pointer to the object
+ *
+ * Return: True if the object needs extra ccs pages. False otherwise.
+ */
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
+{
+ bool lmem_placement = false;
+ int i;
+
+ for (i = 0; i < obj->mm.n_placements; i++) {
+ /* Compression is not allowed for the objects with smem
placement */
+ if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
+ return false;
+ if (!lmem_placement &&
+ obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
+ lmem_placement = true;
+ }
+
+ return lmem_placement;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_DELAYED_WORK(>mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 0bf3ee27a2a8..6f0a3ce35567 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -618,6 +618,8 @@ int i915_gem_object_wait_migration(struct
drm_i915_gem_object *obj,
bool i915_gem_object_placement_possible(struct drm_i915_gem_object *obj,
enum intel_memory_type type);
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj);
+
int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
size_t size, struct intel_memory_region *mr,
struct address_space *mapping,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 098409a33e10..7e1f8b83077f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -266,24 +266,6 @@ static const struct i915_refct_sgt_ops tt_rsgt_ops = {
.release = i915_ttm_tt_release
};
-static inline bool
-i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
-{
- bool lmem_placement = false;
- int i;
-
- for (i = 0; i < obj->mm.n_placements; i++) {
- /* Compression is not allowed for the objects with smem
placement */
- if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
- return false;
- if (!lmem_placement &&
- obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
- lmem_placement = true;
- }
-
- return lmem_placement;
-}
-
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index df14ac81c128..9a7e50534b84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -435,6 +435,9 @@ i915_ttm_memcpy_work_arm(struct i915_ttm_memcpy_work *work,
static bool i915_ttm_memcpy_allowed(struct ttm_buffer_object *bo,
[Intel-gfx] [CI v4 12/13] drm/i915/ttm: disallow CPU fallback mode for ccs pages
Falling back to memcpy/memset shouldn't be allowed if we know we have
CCS state to manage using the blitter. Otherwise we are potentially
leaving the aux CCS state in an unknown state, which smells like an info
leak.
Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs
objects")
Signed-off-by: Matthew Auld
Cc: Thomas Hellström
Cc: Lionel Landwerlin
Cc: Tvrtko Ursulin
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Jordan Justen
Cc: Kenneth Graunke
Cc: Akeem G Abodunrin
Cc: Ramalingam C
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 26
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 18 --
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 +++
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 642a5d59ce26..ccec4055fde3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -717,6 +717,32 @@ bool i915_gem_object_placement_possible(struct
drm_i915_gem_object *obj,
return false;
}
+/**
+ * i915_gem_object_needs_ccs_pages - Check whether the object requires extra
+ * pages when placed in system-memory, in order to save and later restore the
+ * flat-CCS aux state when the object is moved between local-memory and
+ * system-memory
+ * @obj: Pointer to the object
+ *
+ * Return: True if the object needs extra ccs pages. False otherwise.
+ */
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
+{
+ bool lmem_placement = false;
+ int i;
+
+ for (i = 0; i < obj->mm.n_placements; i++) {
+ /* Compression is not allowed for the objects with smem
placement */
+ if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
+ return false;
+ if (!lmem_placement &&
+ obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
+ lmem_placement = true;
+ }
+
+ return lmem_placement;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_DELAYED_WORK(>mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 0bf3ee27a2a8..6f0a3ce35567 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -618,6 +618,8 @@ int i915_gem_object_wait_migration(struct
drm_i915_gem_object *obj,
bool i915_gem_object_placement_possible(struct drm_i915_gem_object *obj,
enum intel_memory_type type);
+bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj);
+
int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
size_t size, struct intel_memory_region *mr,
struct address_space *mapping,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 098409a33e10..7e1f8b83077f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -266,24 +266,6 @@ static const struct i915_refct_sgt_ops tt_rsgt_ops = {
.release = i915_ttm_tt_release
};
-static inline bool
-i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj)
-{
- bool lmem_placement = false;
- int i;
-
- for (i = 0; i < obj->mm.n_placements; i++) {
- /* Compression is not allowed for the objects with smem
placement */
- if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM)
- return false;
- if (!lmem_placement &&
- obj->mm.placements[i]->type == INTEL_MEMORY_LOCAL)
- lmem_placement = true;
- }
-
- return lmem_placement;
-}
-
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index df14ac81c128..9a7e50534b84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -435,6 +435,9 @@ i915_ttm_memcpy_work_arm(struct i915_ttm_memcpy_work *work,
static bool i915_ttm_memcpy_allowed(struct ttm_buffer_object *bo,
struct ttm_resource *dst_mem)
{
+ if (i915_gem_object_needs_ccs_pages(i915_ttm_to_gem(bo)))
+ return false;
+
if (!(i915_ttm_resource_mappable(bo->resource) &&
i915_ttm_resource_mappable(dst_mem)))
return false;
--
2.36.1
