[Intel-gfx] [PATCH 03/15] drm/i915/gt: Use caller provided forcewake for intel_mocs_init_engine

[Tvrtko Ursulin]

On 03/07/2019 12:47, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-03 12:23:52)
>>
>> On 03/07/2019 10:17, Chris Wilson wrote:
>>> During post-reset resume, we call intel_mocs_init_engine to reinitialise
>>> the MOCS registers. Suprisingly, especially when enhanced by lockdep,
>>> the acquisition of the forcewake lock around each register write takes a
>>> substantial portion of the reset time. We don't need to use the
>>> individual forcewake here as we can assume that the caller is holding a
>>> blanket forcewake for the reset&resume and the resume is serialised.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_mocs.c | 15 +++++++++------
>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> index ae6cbf0d517c..290a5e9b90b9 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
>>> @@ -346,6 +346,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>>>        unsigned int index;
>>>        u32 unused_value;
>>>    
>>> +     /* Called under a blanket forcewake */
>>> +     assert_forcewakes_active(uncore, FORCEWAKE_ALL);
>>> +
>>
>> Assert is strictly speaking a bit weak since forcewake status can
>> theoretically change until the actual access below. But in our current
>> code we indeed hold it for the whole reset.
> 
> You want to distinguish between an explicit hold by the caller and the
> auto.

Yes, we just don't have a way of distinguishing if the caller is holding 
or they were held and about to be released.

>> I don't have any actual ideas on how to fundamentally improve the
>> assert. Thought to have it after the writes is the only thing which came
>> to mind. Thoughts?
> 
> I definitely prefer having it upfront to document the function
> preconditions, and so would prefer if the assert actually did assert an
> explicit forcewake as it is meant to do :)

Probably better yes. I was just thinking out loud.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko