Re: [PATCH] drm/i915/skl: Implement DP Aux Mutex framework

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

"aux mutex framework" in the title sounds a bit grandiose, to be
honest. "add support for DP AUX hardware mutex" is what I would have
used.

Further comments inline.

On Thu, 12 Nov 2015, Wayne Boyer <wayne.boyer@intel.com> wrote:
> From: "Boyer, Wayne" <wayne.boyer@intel.com>
>
> Beginning with SKL the DP Aux channel communication can be protected
> using a built in HW mutex.
>
> When PSR is enabled the HW takes control on AUX and uses it to
> control panel exit/entry states.
>
> When validating PSR with automated tests, grabbing CRC from sink
> revealed strange aux communication issues.  Aux reads were returning
> a message read size equal to 0 and 0 is a forbidden message.
>
> By using the HW mutex the HW is blocked from using aux when running
> the automated PSR tests.
>
> This patch provides an initial implementation for using that mutex.
> The use is currently limited to protecting the sink crc request based
> on feedback from the H/W designers indicating that using the mutex
> for all aux channel communication is not recommended.
>
> v2: Improved commit message to explain the case where the HW mutex is
> helpful.  Also added bug reference.
> v3: Fix typos in commit message.
>
> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91437
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_reg.h |  5 ++++
>  drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d5cf30b..ac7ed0d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2585,6 +2585,7 @@ struct drm_i915_cmd_table {
>  
>  #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
> +#define HAS_AUX_MUTEX(dev)	(INTEL_INFO(dev)->gen >= 9)
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev) || \
>  				 IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \
>  				 IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bd2699..f9ee874 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4288,6 +4288,11 @@ enum skl_disp_power_wells {
>  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
>  
> +#define DP_AUX_MUTEX_A			0x6402C
> +#define DP_AUX_MUTEX_B			0x6412C

Please add all of these at once, using an underscore prefix, and add
DP_AUX_MUTEX(port) to pick the right one. Plenty of examples in the
file. And then Ville doesn't have to clean it up afterwards.

> +#define   DP_AUX_MUTEX_ENABLE		    (1 << 31)
> +#define   DP_AUX_MUTEX_STATUS		    (1 << 30)
> +
>  /*
>   * Computing GMCH M and N values for the Display Port link
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index da02ed7..b3c7d82 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -781,6 +781,47 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	       DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
>  }
>  
> +static bool skl_aux_mutex(struct intel_dp *intel_dp, bool get)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t aux_ch_mutex, status;
> +	int count = 0;
> +
> +	if (!HAS_AUX_MUTEX(dev))
> +		return false;
> +
> +	/*
> +	 * FIXME: determine actual aux channel
> +	 * Hard coded to channel A for now to protect sink crc requests on eDP.
> +	 */

If you want to leave it like this for now, the please add "|| !is_edp()"
alongside !HAS_AUX_MUTEX() for an early return.

> +	aux_ch_mutex = DP_AUX_MUTEX_A;
> +
> +	if (!get) {
> +		I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE | DP_AUX_MUTEX_STATUS);
> +		return false;
> +	}
> +
> +	/*
> +	 * The Bspec specifies waiting 500us between attempts to acquire the
> +	 * mutex.  Ten retries should be adequate to balance successfully
> +	 * acquirng the mutex and spending too much time trying.
> +	 */
> +	while (count++ < 10) {

Please use a for loop whenever you can use a for loop. The reader has to
pause for a second to ensure "while (count++ < 10)" does the right thing
and count is initialized properly; "for (i = 0; i < 10; i++)" comes from
the spine.

> +		I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE);
> +		status = I915_READ(aux_ch_mutex);
> +		if (!(status & DP_AUX_MUTEX_STATUS))
> +			return true;
> +		udelay(500);
> +	}

You should probably add an error or a debug message here.

I observe that the caller has no way of knowing whether the lock was not
acquired because the platform doesn't support it, or because we failed
to acquire it. I would imagine the callers might want to opt for not
proceeding to mess with the hardware if they can't ensure exclusive
access. That's kind of the point of locking.

> +
> +	return false;
> +}
> +
> +#define skl_aux_mutex_get(dev) skl_aux_mutex(dev, true)
> +#define skl_aux_mutex_put(dev) skl_aux_mutex(dev, false)

Please have two separate functions instead of this indirection. Name
them _lock and _unlock; _get and _put imply support for
refcounting/nesting, which isn't there.

> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -3927,10 +3968,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	u8 buf;
>  	int count, ret;
>  	int attempts = 6;
> +	bool aux_mutex_acquired = false;

Unnecessary initialization.

> +
> +	aux_mutex_acquired = skl_aux_mutex_get(intel_dp);
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
> +

Unwanted whitespace change.

>  	if (ret)
> -		return ret;
> +		goto release;
>  
>  	do {
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> @@ -3957,6 +4002,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  
>  stop:
>  	intel_dp_sink_crc_stop(intel_dp);
> +
> +release:
> +	if (aux_mutex_acquired)
> +		aux_mutex_acquired = skl_aux_mutex_put(intel_dp);

No need to do an assignment here.

BR,
Jani.


> +
>  	return ret;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx