Re: [Intel-gfx] [PATCH v2 5/6] drm/i915: Don't back up pinned LMEM context images and rings during suspend

[Matthew Auld <matthew.auld@intel.com>]

On 08/09/2021 13:26, Thomas Hellström wrote:
> On Wed, 2021-09-08 at 12:07 +0100, Matthew Auld wrote:
>> On 06/09/2021 17:55, Thomas Hellström wrote:
>>> Pinned context images are now reset during resume. Don't back them
>>> up,
>>> and assuming that rings can be assumed empty at suspend, don't back
>>> them
>>> up either.
>>>
>>> Introduce a new object flag, I915_BO_ALLOC_PM_VOLATILE meaning that
>>> an
>>> object is allowed to lose its content on suspend.
>>>
>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> ---
>>>    .../gpu/drm/i915/gem/i915_gem_object_types.h    | 17 ++++++++++--
>>> -----
>>>    drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c      |  3 +++
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c             |  3 ++-
>>>    drivers/gpu/drm/i915/gt/intel_ring.c            |  3 ++-
>>>    4 files changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> index 734cc8e16481..66123ba46247 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> @@ -288,16 +288,19 @@ struct drm_i915_gem_object {
>>>          I915_SELFTEST_DECLARE(struct list_head st_link);
>>>    
>>>          unsigned long flags;
>>> -#define I915_BO_ALLOC_CONTIGUOUS BIT(0)
>>> -#define I915_BO_ALLOC_VOLATILE   BIT(1)
>>> -#define I915_BO_ALLOC_CPU_CLEAR  BIT(2)
>>> -#define I915_BO_ALLOC_USER       BIT(3)
>>> +#define I915_BO_ALLOC_CONTIGUOUS  BIT(0)
>>> +#define I915_BO_ALLOC_VOLATILE    BIT(1)
>>> +#define I915_BO_ALLOC_CPU_CLEAR   BIT(2)
>>> +#define I915_BO_ALLOC_USER        BIT(3)
>>> +/* Object may lose its contents on suspend / resume */
>>> +#define I915_BO_ALLOC_PM_VOLATILE BIT(4)
> 
>>
>> PM_SKIP_PINNED? Not sure if that is better.
> 
> I think we could update the comment to say "object is allowed to
> lose..", I think we could keep PM_VOLATILE to keep it consistent with
> the ALLOC_VOLATILE flag?

I guess that's the potentially confusing bit. ALLLOC_VOLATILE means the 
pages might be discarded as soon as the pages become unpinned, without 
needing to worry about persisting their contents. With PM_VOLATILE I was 
expecting something similar where unpinned objects can simply be skipped 
or ignored during pm. Anyway, that's just a bikeshed, I think with 
improved comment this should be fine.

> 
> /Thomas
> 
>