[Intel-gfx] [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
Souza, Jose
jose.souza at intel.com
Wed Feb 28 23:55:39 UTC 2018
On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > by it
> > > > self, so lets use the mutex register that is available in gen9+
> > > > to
> > > > avoid concurrent access by hardware and driver.
> > > > Older gen handling will be done separated.
> > > >
> > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > el-g
> > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > >
> > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com
> > > > >
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >
> > > > Changelog:
> > > > v2
> > > > - removed the PSR dependency, now getting lock all the times
> > > > when
> > > > available
> > > > - renamed functions to avoid nested calls
> > > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > > Dhinakaran Pandiyan will sent a better and final version
> > > > v3
> > > > - rebased on top of Ville's AUX series
> > > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > > - using intel_wait_for_register() instead of the internal
> > > > version
> > > > v4
> > > > - removed virtual function to get mutex register address
> > > > - enabling the mutex back only on aux channel init
> > > > - added the aux channel name to the timeout debug message
> > > > v5
> > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > >
> > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 56 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5385,6 +5385,15 @@ enum {
> > > > #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 _DPA_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6402C)
> > > >
> > > > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6412C)
> > > >
> > > > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6422C)
> > > >
> > > > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6432C)
> > > >
> > > > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6452C)
> > > >
> > > > +#define DP_AUX_CH_MUTEX(aux_ch) _MMIO_PORT(aux_ch,
> > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > > +#define DP_AUX_CH_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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > aux_clock_divi
> > > > der)
> > > > ;
> > > > }
> > > >
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return true;
> > > > +
> > > > + /* Spec says that mutex is acquired when status bit is
> > > > read as unset,
> > > > + * here waiting for 2msec(+-4 aux transactions) before
> > > > give up.
> > > > + */
> > > > + if (intel_wait_for_register(dev_priv,
> > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > > 2))
> > > > {
> > > > + DRM_DEBUG_KMS("aux channel %c locked for
> > > > 2msec,
> > > > timing out\n",
> > > > + aux_ch_name(intel_dp->aux_ch));
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return;
> > > > +
> > > > + /* set the status bit releases the mutex + keeping
> > > > mutex
> > > > enabled */
> > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_ENABLE |
> > > > DP_AUX_CH_MUTEX_STATUS);
> > > > +}
> > > > +
> > > > static int
> > > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > const uint8_t *send, int send_bytes,
> > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > intel_dp_check_edp(intel_dp);
> > > >
> > > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > + ret = -EBUSY;
> > > > + goto out_aux_ch_unlocked;
> > > > + }
> > > > +
> > > > /* Try to wait for any previous AUX channel activity
> > > > */
> > > > for (try = 0; try < 3; try++) {
> > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > ret = recv_bytes;
> > > > out:
> > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_aux_ch_unlocked:
> > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >
> > > > if (vdd)
> > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
> > > > *intel_dp)
> > > > else
> > > > intel_dp->get_aux_send_ctl =
> > > > g4x_get_aux_send_ctl;
> > > >
> > > > + if (INTEL_GEN(dev_priv) >= 9)
> > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_ENABLE);
> > >
> > > And who enables it after system/runtime PM etc.? Not sure how
> > > much
> > > sense
> > > there is to finesse this anyway. One extra mmio write every time
> > > we
> > > try
> > > to acquire the mutex shouldn't kill anyone.
> >
> > Valid point, but I guess is better enable the mutex in
> > intel_dp_encoder_reset():
> >
> > "
> > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > drm_encoder
> > *encoder)
> >
> > pps_lock(intel_dp);
> >
> > + if (INTEL_GEN(dev_priv) >= 9)
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_ENABLE);
> > +
> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > "
> >
> > any objections?
>
> I guess we can hope that dmc takes care of it for dc5/6. But what
> about dc9?
Yep, for DC5 and DC6 the registers are restored by hardware but it is
not the case for DC9. If I'm not missing something looks like there is
no handling to restore registers when exiting DC9.
>
More information about the Intel-gfx
mailing list