On Mon, 2018-02-26 at 18:31 +0000, Souza, Jose wrote:
> 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.v...@intel.com>
> > > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> > > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.so...@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,
I think the status bit will still indicate busy if the HW started the
aux transaction before mutex was enabled. I think so because bspec
explicitly says to check the status bit right after enabling.
> 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.
>
Might be worth checking how this plays with debug messages in the upper
layers.
> > 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.
Limiting the scope for changes to where they are required reduces bugs
IMO. However, if you want to do this unconditionally (without checking
for PSR) on all ports, aux_init() that Ville added might be a good
place.
>
> >
> > 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(),
Doesn't this confirm my guess that enabling mutex per transaction is
okay? We'll still see status as 1.
Anyway, since we are leaning towards the "always enable mutex
approach".Please remove the enable step for every single transaction
that the driver initiates.
If the hardware had already started an aux transaction, status bit will
be 1. Now, if you go overwrite the register to enable the mutex, which
is already enabled second time onwards, you'll end-up 0'ing the status
bit.
> 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);
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
