Re: [PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

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

On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Some display MMIO transactions for offsets in the range that requires
> the DMC wakelock happen in atomic context (this has been confirmed
> during tests on PTL). That means that we need to use a non-sleeping
> variant of MMIO waiting function.
>
> Implement __intel_de_wait_for_register_atomic_nowl() and use it when
> waiting for acknowledgment of acquire/release.
>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index e017cd4a8168..4116783a62dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
>  }
>  #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
>  
> +static inline int
> +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
> +					   i915_reg_t reg,
> +					   u32 mask, u32 value,
> +					   unsigned int fast_timeout_us)
> +{
> +	return __intel_wait_for_register(__to_uncore(display), reg, mask,
> +					 value, fast_timeout_us, 0, NULL);
> +}
> +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
> +

There's no need to add the wrapper when all users pass struct
intel_display. And we don't want new users that pass i915.

And why are we adding new stuff and users with double underscores?

>  static inline int
>  __intel_de_wait(struct intel_display *display, i915_reg_t reg,
>  		u32 mask, u32 value, unsigned int timeout)
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 5634ff07269d..8056a3c8666c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -39,7 +39,7 @@
>   * potential future use.
>   */
>  
> -#define DMC_WAKELOCK_CTL_TIMEOUT 5
> +#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000
>  #define DMC_WAKELOCK_HOLD_TIME 50
>  
>  struct intel_dmc_wl_range {
> @@ -78,9 +78,9 @@ static void intel_dmc_wl_work(struct work_struct *work)
>  
>  	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
>  
> -	if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
> -					      DMC_WAKELOCK_CTL_ACK, 0,
> -					      DMC_WAKELOCK_CTL_TIMEOUT)) {
> +	if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
> +						     DMC_WAKELOCK_CTL_ACK, 0,
> +						     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>  		WARN_RATELIMIT(1, "DMC wakelock release timed out");
>  		goto out_unlock;
>  	}
> @@ -216,10 +216,14 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>  		__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
>  				    DMC_WAKELOCK_CTL_REQ);
>  
> -		if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
> -						      DMC_WAKELOCK_CTL_ACK,
> -						      DMC_WAKELOCK_CTL_ACK,
> -						      DMC_WAKELOCK_CTL_TIMEOUT)) {
> +		/*
> +		 * We need to use the atomic variant of the waiting routine
> +		 * because the DMC wakelock is also taken in atomic context.
> +		 */
> +		if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
> +							     DMC_WAKELOCK_CTL_ACK,
> +							     DMC_WAKELOCK_CTL_ACK,
> +							     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>  			WARN_RATELIMIT(1, "DMC wakelock ack timed out");
>  			goto out_unlock;
>  		}

-- 
Jani Nikula, Intel