Re: [CI v4 12/13] drm/i915/ttm: disallow CPU fallback mode for ccs pages

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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 <matthew.auld@xxxxxxxxx>
> > > Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > > Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: Jordan Justen <jordan.l.justen@xxxxxxxxx>
> > > Cc: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> > > Cc: Akeem G Abodunrin <akeem.g.abodunrin@xxxxxxxxx>
> > > Cc: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > > ---
> > >   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(&i915->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;