[Intel-gfx] [PATCH 1/7] drm/i915: Lift runtime-pm acquire callbacks out of intel_wakeref.mutex

[Nirmoy Das]

Hi Jani,

On 9/20/2023 11:03 AM, Jani Nikula wrote:
> On Mon, 18 Sep 2023, Nirmoy Das <nirmoy.das at intel.com> wrote:
>> From: Chris Wilson <chris.p.wilson at intel.com>
>>
>> When runtime pm is first woken, it will synchronously call the
>> registered callbacks for the device. These callbacks
>> may pull in their own forest of locks, which we do not want to
>> conflate with the intel_wakeref.mutex. A second minor benefit to
>> reducing the coverage of the mutex, is that it will reduce
>> contention for frequent sleeps and wakes (such as when being used
>> for soft-rc6).
>>
>> Signed-off-by: Chris Wilson <chris.p.wilson at intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
> Is this patch a dependency on the subsequent patches in the series? If
> yes, what's the rationale?

When we do GGTT update with gpu request, we need to take pm wakeref for 
GT as well as for the engine and

without this patch/improvement there will be lockdep warning as we are 
not allowed to alloc memory with

vm->mutex held which can happen when the runtime pm woken up 1st.


> If not, please submit separately. None of
> this is is obvious in the commit messages.


So I would like to keep this patch in the series. I will add rationale 
in the cover letter and

resend with your below suggestions.


Thanks,

Nirmoy

>
>> ---
>>   drivers/gpu/drm/i915/intel_wakeref.c | 43 ++++++++++++++--------------
>>   1 file changed, 21 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
>> index 718f2f1b6174..af7b4cb5b4d7 100644
>> --- a/drivers/gpu/drm/i915/intel_wakeref.c
>> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
>> @@ -10,21 +10,11 @@
>>   #include "intel_wakeref.h"
>>   #include "i915_drv.h"
>>   
>> -static void rpm_get(struct intel_wakeref *wf)
>> -{
>> -	wf->wakeref = intel_runtime_pm_get(&wf->i915->runtime_pm);
>> -}
>> -
>> -static void rpm_put(struct intel_wakeref *wf)
>> -{
>> -	intel_wakeref_t wakeref = fetch_and_zero(&wf->wakeref);
>> -
>> -	intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
>> -	INTEL_WAKEREF_BUG_ON(!wakeref);
>> -}
>> -
>>   int __intel_wakeref_get_first(struct intel_wakeref *wf)
>>   {
>> +	intel_wakeref_t wakeref = intel_runtime_pm_get(&wf->i915->runtime_pm);
> No non-trivial function calls in the initializer please.
>
>> +	int err = 0;
> Until now err was only for handling error returns. If it's also for
> returning success, it should probably be named ret.
>
>> +
>>   	/*
>>   	 * Treat get/put as different subclasses, as we may need to run
>>   	 * the put callback from under the shrinker and do not want to
>> @@ -32,41 +22,50 @@ int __intel_wakeref_get_first(struct intel_wakeref *wf)
>>   	 * upon acquiring the wakeref.
>>   	 */
>>   	mutex_lock_nested(&wf->mutex, SINGLE_DEPTH_NESTING);
>> -	if (!atomic_read(&wf->count)) {
>> -		int err;
>>   
>> -		rpm_get(wf);
>> +	if (likely(!atomic_read(&wf->count))) {
> Adding the likely should be a separate patch with rationale, not a
> random drive-by change. (And maybe it just should not be added at all.)
>
>> +		INTEL_WAKEREF_BUG_ON(wf->wakeref);
>> +		wf->wakeref = fetch_and_zero(&wakeref);
> fetch_and_zero() should just die. All it does here is make things more
> confusing, not less. Please don't add new users.
>
> The get and put helpers could probably stay, modified, to make this more
> readable.
>
>>   
>>   		err = wf->ops->get(wf);
>>   		if (unlikely(err)) {
>> -			rpm_put(wf);
>> -			mutex_unlock(&wf->mutex);
>> -			return err;
>> +			wakeref = xchg(&wf->wakeref, 0);
>> +			wake_up_var(&wf->wakeref);
> e.g. here this bit is duplicated in ____intel_wakeref_put_last().
>
>> +			goto unlock;
>>   		}
>>   
>>   		smp_mb__before_atomic(); /* release wf->count */
>>   	}
>>   	atomic_inc(&wf->count);
>> +	INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
>> +
>> +unlock:
>>   	mutex_unlock(&wf->mutex);
>> +	if (unlikely(wakeref))
>> +		intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
>>   
>> -	INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
>> -	return 0;
>> +	return err;
>>   }
>>   
>>   static void ____intel_wakeref_put_last(struct intel_wakeref *wf)
>>   {
>> +	intel_wakeref_t wakeref = 0;
>> +
>>   	INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
>>   	if (unlikely(!atomic_dec_and_test(&wf->count)))
>>   		goto unlock;
>>   
>>   	/* ops->put() must reschedule its own release on error/deferral */
>>   	if (likely(!wf->ops->put(wf))) {
>> -		rpm_put(wf);
>> +		INTEL_WAKEREF_BUG_ON(!wf->wakeref);
>> +		wakeref = xchg(&wf->wakeref, 0);
>>   		wake_up_var(&wf->wakeref);
>>   	}
>>   
>>   unlock:
>>   	mutex_unlock(&wf->mutex);
>> +	if (wakeref)
>> +		intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
>>   }
>>   
>>   void __intel_wakeref_put_last(struct intel_wakeref *wf, unsigned long flags)