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

[Tvrtko Ursulin]

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 at intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>>> Cc: Akeem G Abodunrin <akeem.g.abodunrin at intel.com>
>>> Cc: Ramalingam C <ramalingam.c at intel.com>
>>> ---
>>>   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;