[Intel-gfx] [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
Rodrigo Vivi
rodrigo.vivi at intel.com
Wed Feb 21 22:28:12 UTC 2018
On Wed, Feb 21, 2018 at 01:12:08PM -0800, Souza, Jose wrote:
> On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 20, 2018 at 06:23:47PM -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.
> > >
> > > Reference: https://01.org/sites/default/files/documentation/intel-g
> > > fx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> > >
> >
> > Cc: Ville
> > Cc: DK
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Jani Nikula <jani.nikula 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 | 50
> > > ++++++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > 3 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..a62e3c1badab 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5318,6 +5318,7 @@ enum {
> > > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6401c)
> > > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64020)
> > > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64024)
> > > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6402C)
> > >
> > > #define _DPB_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64110)
> > > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64114)
> > > @@ -5325,6 +5326,7 @@ enum {
> > > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6411c)
> > > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64120)
> > > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64124)
> > > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6412C)
> > >
> > > #define _DPC_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64210)
> > > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64214)
> > > @@ -5332,6 +5334,7 @@ enum {
> > > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6421c)
> > > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64220)
> > > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64224)
> > > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6422C)
> > >
> > > #define _DPD_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64310)
> > > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64314)
> > > @@ -5339,6 +5342,7 @@ enum {
> > > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6431c)
> > > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64320)
> > > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64324)
> > > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6432C)
> > >
> > > #define _DPF_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64510)
> > > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64514)
> > > @@ -5346,6 +5350,7 @@ enum {
> > > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6451c)
> > > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64520)
> > > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64524)
> > > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6452C)
> > >
> > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> > > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > @@ -5378,6 +5383,10 @@ 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)
> > >
> >
> >
> > I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined
> > here along
> > with the bits as Jani suggested.
> > I don't see any reason to tag that far from the bits and close to CTL
> > and DATA.
> >
> > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > > _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 f20b25f98e5a..af07563bafba 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);
> > > +
> > > + if (INTEL_GEN(dev_priv) < 9)
> > > + return true;
> > > +
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > 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.
>
> >
> > > +
> > > + /* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > + * if set it should be read again after 500usec. Here
> > > waiting for
> > > + * 2msec or 4 tries before give up.
> > > + */
> > > + if (__intel_wait_for_register(dev_priv, intel_dp-
> > > >aux_ch_mutex_reg,
> > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > 500, 2,
> > > + NULL)) {
> >
> > why not intel_wait_for_register?
> > that seems cleaner...
>
> intel_wait_for_register() will call __intel_wait_for_register() with
> fast_timeout_us set as 2usec and specs says we should wait 500usec.
So, I've just noticed that __intel_wait_for_register spreaded to intel_hdcp.
Ideally if we have __ prefix this shouldn't be called for more than 1 place.
So it seems that we have more cases hence a rework should be done here.
But also the fast case is the atomic initial case... it is the first read.
Only if it fails it will consider the slow case. That's exactly what spec tells
Maybe what we want here is a intel_wait_for_register_us to have a slow value
based in us rather than ms?
I'm not fully sure here.
>
> >
> > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > + 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;
> > > +
> > > + /* setting the status bit releases the mutex + keep mutex
> > > enabled */
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE |
> > > + DP_AUX_CH_MUTEX_STATUS);
> >
> > ditto.
> >
> > > +}
> > > +
> > > static int
> > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > const uint8_t *send, int send_bytes,
> > > @@ -1115,6 +1154,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;
> > > + }
> > > +
> >
> > BSpec: 4301
> > I believe you could reduce the lock region following the spec
> > and limiting it to steps 5-to-10, i.e.
> > inside the loop below.
>
> We loop trying to complete the transaction but we copy data received
> out of the loop and that is step 9, so the lock must be held until we
> copy the data from intel_dp->aux_ch_data_reg[] to recv.
>
> "
> for (i = 0; i < recv_bytes; i += 4)
> intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >>
> 2]), recv + i, recv_bytes - i);
>
> ret = recv_bytes;
> out:
> intel_dp_aux_ch_unlock(intel_dp);
> "
hm... ok... this is the actually part 9 that needs to be inside
the locked area indeed. I missed that.
>
> >
> > > /* Try to wait for any previous AUX channel activity */
> > > for (try = 0; try < 3; try++) {
> > > status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1244,6 +1288,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)
> > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > > intel_dp *intel_dp)
> > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > > port);
> > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > > i++)
> > > intel_dp->aux_ch_data_reg[i] =
> > > intel_aux_data_reg(dev_priv, port, i);
> > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > + intel_dp->aux_ch_mutex_reg =
> > > DP_AUX_CH_MUTEX(port);
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> >
> > I don't believe we need an extra enable here. And hopefully with
> > Villes
> > rework we don't even need to define here but only accessing it
> > directly from
> > the aux function with intel_dp->aux_ch reference.
>
> What if hardware wants to do some aux transaction before we do our
> first transaction(and enabling the mutex)? The mutex will not be
> enabled so hardware will not lock it.
>
> >
> > > + }
> > > }
> > >
> > > static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 50874f4035cf..0c88657d6066 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > i915_reg_t output_reg;
> > > i915_reg_t aux_ch_ctl_reg;
> > > i915_reg_t aux_ch_data_reg[5];
> > > + i915_reg_t aux_ch_mutex_reg;
> > > uint32_t DP;
> > > int link_rate;
> > > uint8_t lane_count;
> > > --
> > > 2.16.2
> > >
More information about the Intel-gfx
mailing list