[Intel-gfx] [PATCH] drm/i915/skl: Implement DP Aux Mutex framework
Jani Nikula
jani.nikula at linux.intel.com
Thu Nov 12 02:16:51 PST 2015
"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 at intel.com> wrote:
> From: "Boyer, Wayne" <wayne.boyer at 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 at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91437
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Tested-by: Rodrigo Vivi <rodrigo.vivi at 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
More information about the Intel-gfx
mailing list