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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 18 Sep 2023, Nirmoy Das <nirmoy.das@intel.com> wrote:
> From: Chris Wilson <chris.p.wilson@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@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Is this patch a dependency on the subsequent patches in the series? If
yes, what's the rationale? If not, please submit separately. None of
this is is obvious in the commit messages.

> ---
>  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)

-- 
Jani Nikula, Intel