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

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

On Tue, 10 Nov 2015, Wayne Boyer <wayne.boyer@intel.com> wrote:
> Beginning with SKL the DP Aux channel communication can be protected
> using a built in H/W mutex.
>
> 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.

Please explain in the commit message what benefits this brings, and why
it's a good idea. Just that something *can* be done in hardware does not
mean that we should do it.

BR,
Jani.


>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@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 b12594b..98e991d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2604,6 +2604,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 8942532..a033e70 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
> +#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 2fad873..e9e1239 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.
> +	 */
> +	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) {
> +		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);
> +	}
> +
> +	return false;
> +}
> +
> +#define skl_aux_mutex_get(dev) skl_aux_mutex(dev, true)
> +#define skl_aux_mutex_put(dev) skl_aux_mutex(dev, false)
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -4188,10 +4229,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;
> +
> +	aux_mutex_acquired = skl_aux_mutex_get(intel_dp);
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
> +
>  	if (ret)
> -		return ret;
> +		goto release;
>  
>  	do {
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> @@ -4218,6 +4263,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);
> +
>  	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