[Intel-gfx] [PATCH 1/1] drm/i915/skl+: Add and enable DP AUX CH mutex
Souza, Jose
jose.souza at intel.com
Mon Feb 26 18:31:12 UTC 2018
On Fri, 2018-02-23 at 19:12 -0800, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-23 at 17:51 -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/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++
> > drivers/gpu/drm/i915/intel_dp.c | 67
> > ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 77 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..f36e839b4b4f 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(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>
> ^aux_ch similar to ctl and data.
>
> > +#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 2c3eb90b9499..7be2fec51651 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider)
> > ;
> > }
> >
> > +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);
> > + i915_reg_t ch_mutex;
> > +
> > + if (!intel_dp->aux_ch_mutex_reg)
> > + return true;
> > +
> > + ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> > + I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE);
>
>
> > > >
> > > > You might be touching bits. We don't know if HW is using the
> > > > reserved
> > > > bits or not.
> > > > So RMW |= bit 31 here is a good idea.
> > >
> > > As a read in this register request the mutex lock is better avoid
> > > any
> > > read that is not meant to request it.
> >
> > ok... I accept the fact that read that is locking
> > so you are right here.
> >
>
> I do not agree with the interpretation here, reading the register
> *after* the mutex is enabled == request for locking. You can read the
> register before enabling, and you have to read so that you don't
> overwrite any other bit.
>
> Ref: "Sticky bit set to 1 after a read to this register when Mutex is
> enabled."
This is true but we must keep the mutex enabled all the time to
guarantee that hardware will request lock too, so any read to the
register from our side will request the lock.
>
>
> > +
> > + /* Spec says that mutex is acquired when status bit is
> > read as unset,
> > + * here waiting for 2msec or 4 tries before give up.
>
> 2 ms. ^ this is not true
>
> > + */
> > + if (intel_wait_for_register(dev_priv, ch_mutex,
> > DP_AUX_CH_MUTEX_STATUS,
> > + 0, 2)) {
> > + DRM_WARN("dp_aux_ch port locked for too long");
>
> DRM_DEBUG_KMS("aux channel %c locked for 2 ms, timing
> out\n");
>
> 1. DRM_DEBUG_KMS is the convention in this file and most parts of the
> driver for things like this.
I'm okay in change I was just using DRM_WARN because most of the
messages in intel_dp_aux_ch() is using it.
> 2. I prefer to add details like port/channel/pipe/connector when
> printing debug messages if it doesn't cost any extra space.
>
>
>
> > + 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_dp->aux_ch_mutex_reg)
> > + return;
> > +
> > + /* setting the status bit releases the mutex + keep mutex
> > enabled */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
> > + DP_AUX_CH_MUTEX_ENABLE |
> > DP_AUX_CH_MUTEX_STATUS);
>
> If you are leaving the mutex enabled, you don't have to enable it for
> the second transaction onwards. In that case, why not move the mutex
> enabling step to intel_dp_aux_init() after checking for PSR support
> or
> enable the mutex just before intel_psr_enable(). Please get an
> alternate
> opinion on this.
>
>
> I think there are two options:
> 1)
> Enable mutex before psr_enable
> Read status (== request for lock)
> aux_xfer
> Write status(== release lock)
> Disable mutex after psr_disable
The mutex will also be needed when enabling GTC and the cost of always
leave enabled is so low that is not worthy check if we have PSR enabled
in the port.
>
> 2)
> For every aux transaction.
> Enable mutex (RMW is okay because mutex is not enabled)
> Read status (== request for lock)
> aux_xfer
> Write status(== release lock)
> Disable mutex
Enable at each aux transaction is wrong, this way hardware will not
request the lock when it is disabled.
>
> As of now, you end up writing to the status bit in
> intel_dp_aux_ch_trylock(), which looks wrong.
Vivi belives tha we need to poke registers before read it, in my tests
I can detect that hardware is helding the mutex with or without
asserting enable bit in intel_dp_aux_ch_trylock(), I'm fine with both
ways.
>
>
> -DK
>
>
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1119,6 +1158,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_locked;
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1248,6 +1292,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > ret = recv_bytes;
> > out:
> > + intel_dp_aux_ch_unlock(intel_dp);
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct
> > intel_dp *intel_dp, int index)
> > }
> > }
> >
> > +static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv =
> > to_i915(intel_dp_to_dev(intel_dp));
> > + enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> > + switch (aux_ch) {
> > + case AUX_CH_A:
> > + case AUX_CH_B:
> > + case AUX_CH_C:
> > + case AUX_CH_D:
> > + case AUX_CH_F:
> > + return DP_AUX_CH_MUTEX(aux_ch);
> > + default:
> > + MISSING_CASE(aux_ch);
> > + return DP_AUX_CH_MUTEX(AUX_CH_A);
> > + }
> > +}
> > +
> > static void
> > intel_dp_aux_fini(struct intel_dp *intel_dp)
> > {
> > @@ -1544,6 +1608,9 @@ 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)
> > + intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
> > +
>
> Move this to the branch where control and data vfuncs are setup.
>
> > drm_dp_aux_init(&intel_dp->aux);
> >
> > /* Failure to allocate our preferred name is not critical
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 8f38e584d375..267cc6c5a89f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1128,6 +1128,7 @@ struct intel_dp {
> >
> > i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> > i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int
> > index);
> > + i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
> >
> > /* This is called before a link training is starterd */
> > void (*prepare_link_retrain)(struct intel_dp *intel_dp);
More information about the Intel-gfx
mailing list