Re: [PATCH] drm/i915/ttm: don't leak the ccs state

[Matthew Auld]

On 28/07/2022 00:16, C, Ramalingam wrote:
-----Original Message-----
From: Auld, Matthew <matthew.a...@intel.com>
Sent: Wednesday, July 27, 2022 10:14 PM
To: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org; Thomas Hellström 
<thomas.hellst...@linux.intel.com>; C,
Ramalingam <ramalinga...@intel.com>
Subject: [PATCH] drm/i915/ttm: don't leak the ccs state
The kernel only manages the ccs state with lmem-only objects, however the 
kernel should still take
care not to leak the CCS state from the previous user.

Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs 
objects")
Signed-off-by: Matthew Auld <matthew.a...@intel.com>
Cc: Thomas Hellström <thomas.hellst...@linux.intel.com>
Cc: Ramalingam C <ramalinga...@intel.com>
---
  drivers/gpu/drm/i915/gt/intel_migrate.c | 23 ++++++++++++++++++++++-
  1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c 
b/drivers/gpu/drm/i915/gt/intel_migrate.c
index a69b244f14d0..9a0814422ba4 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -708,7 +708,7 @@ intel_context_migrate_copy(struct intel_context *ce,
        u8 src_access, dst_access;
        struct i915_request *rq;
        int src_sz, dst_sz;
-       bool ccs_is_src;
+       bool ccs_is_src, overwrite_ccs;
        int err;

        GEM_BUG_ON(ce->vm != ce->engine->gt->migrate.context->vm);
@@ -749,6 +749,8 @@ intel_context_migrate_copy(struct intel_context *ce,
                        get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
        }

+       overwrite_ccs = HAS_FLAT_CCS(i915) && !ccs_bytes_to_cpy &&
+dst_is_lmem;
+
        src_offset = 0;
        dst_offset = CHUNK_SZ;
        if (HAS_64K_PAGES(ce->engine->i915)) { @@ -852,6 +854,25 @@
intel_context_migrate_copy(struct intel_context *ce,
                        if (err)
                                goto out_rq;
                        ccs_bytes_to_cpy -= ccs_sz;
+               } else if (overwrite_ccs) {
+                       err = rq->engine->emit_flush(rq, EMIT_INVALIDATE);
+                       if (err)
+                               goto out_rq;
+
+                       /*
+                        * While we can't always restore/manage the CCS state,
+                        * we still need to ensure we don't leak the CCS state
+                        * from the previous user, so make sure we overwrite it
+                        * with something.
+                        */
+                       err = emit_copy_ccs(rq, dst_offset, INDIRECT_ACCESS,
+                                           dst_offset, DIRECT_ACCESS, len);
+                       if (err)
+                               goto out_rq;
+
+                       err = rq->engine->emit_flush(rq, EMIT_INVALIDATE);
+                       if (err)
+                               goto out_rq;
The change is looking good to the purpose. But shouldn't this be the part of 
lmem allocation itself?

Hmm, what do you mean by the lmem allocation itself? The scenarios I had 
in mind here were:

- { lmem, smem } buffer, object is allocated in smem (like with initial 
mmap) and then moved to lmem (smem -> lmem).

- { lmem, smem } buffer, object is allocated in lmem, but then evicted 
to smem. Object then moved back to lmem (smem -> lmem).

- { lmem, smem} buffer with CPU_ACCESS flag on small-bar system, object 
is allocated in non-mappable lmem, and them moved to the mappable part 
of lmem on fault (lmem -> lmem).

In all the above cases the CCS state is left uninitialised, AFAICT.


Ram.
                }

                /* Arbitration is re-enabled between requests. */
--
2.37.1