[PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
Rodrigo Vivi
rodrigo.vivi at intel.com
Tue Feb 13 21:13:48 UTC 2018
On Tue, Feb 13, 2018 at 08:43:26PM +0000, Souza, Jose wrote:
> Thanks Vivi, answers bellow.
>
> On Mon, 2018-02-12 at 23:23 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 13, 2018 at 12:16:09AM +0000, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2 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.
> > >
> > > 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 | 35
> > > +++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
> > > 4 files changed, 51 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index e9c79b560823..d77770ce5728 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5291,6 +5291,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)
> > > @@ -5298,6 +5299,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)
> > > @@ -5305,6 +5307,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)
> > > @@ -5312,6 +5315,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)
> > > @@ -5319,9 +5323,11 @@ 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 */
> > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > >
> > > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > > @@ -5351,6 +5357,9 @@ 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 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 abbe1e4e0af5..c476af39f69f 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1115,6 +1115,30 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > intel_dp_check_edp(intel_dp);
> > >
> > > + /* Get hardware mutex when PSR is enabled */
> >
> > what about creating two functions
> > bool intel_dp_aux_get
> > void intel_dp_aux_put ?
>
> sounds good
>
> >
> > > + if (dev_priv->psr.enabled == intel_dp &&
> >
> > if (dev_priv->psr.enabled) is enough
> >
> > > + intel_dp->aux_ch_mutex_reg.reg) {
> > > + for (try = 0; try < 3; try++) {
> > > + status = I915_READ_NOTRACE(intel_dp-
> > > >aux_ch_mutex_reg);
> > > + if ((status & DP_AUX_CH_MUTEX_STATUS) ==
> > > 0)
> > > + break;
> > > + udelay(500);
> >
> > what about using wait_for(mutex, STATUS, 0, 500) ?
>
> thanks
>
> >
> > > + }
> > > +
> > > + if (try == 3) {
> > > + DRM_WARN("dp_aux_ch port locked for too
> > > long");
> > > + ret = -EBUSY;
> > > + goto out_locked;
> > > + }
> > > + }
> >
> > I don't believe we need to retry...
> > I hope we don't...
> > so if wait_for failed debug_error and return false...
>
> hardware can lock the aux channel and do more than one transaction also
> one transaction can last more than 500usec so we need to retry.
>
> >
> > > +
> > > + /* Warn on when PSR is enabled and GPU gen don't support
> > > AUX CH mutex,
> > > + * hardware and software can be using AUX CH at the same
> > > time in this
> > > + * condition.
> > > + */
> > > + WARN_ON(dev_priv->psr.enabled == intel_dp &&
> > > + !intel_dp->aux_ch_mutex_reg.reg);
> >
> > we don't want to be noise about something that we know already.
> > on those cases we should disable the psr on aux transactions,
> > but warn_on is not a good use here...
>
> I would remove this warning in another patch were I would address this
> case but I will remove it from this one.
>
> >
> > > +
> > > /* Try to wait for any previous AUX channel activity */
> > > for (try = 0; try < 3; try++) {
> > > status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1243,7 +1267,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > recv + i, recv_bytes - i);
> > >
> > > ret = recv_bytes;
> > > +
> >
> > spurious line here...
> >
> > > out:
> > > + if (dev_priv->psr.enabled == intel_dp &&
> > > + intel_dp->aux_ch_mutex_reg.reg) {
> > > + /* setting the status bit releases the mutex */
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE |
> >
> > enable bit is optional here... I believe it gets cleaner if we only
> > set status bit using RMW
>
> but if the mutex_enable is not kept set hardware will not get the mutex
> lock
HW knows it...
Same link that you pasted below states that
"Release with (is optional) or without disabling MUTEX function."
in other words, "enable" means SW is trying to grab the lock.
>
> >
> > btw I just noticed that you are missing the write enable on the get
> > part, before checking for status change.
> > also a RMW there.
>
> According to spec(https://gfxspecs.intel.com/Predator/Home/Index/4301)
> to get the lock we just need to read the register while mutex_enable
> bit is set and check the mutex_status bit.
That's not my understanding from the same page.
I see the get as 2 steps:
3. Enable MUTEX without changing MUTEX status
4. Read MUTEX status. If MUTEX status is '1', wait for 500 us and poll for MUTEX status == '0'.
>
> >
> > > + DP_AUX_CH_MUTEX_STATUS);
> > > + }
> > > +
> > > +out_locked:
> >
> > maybe we could even reduce the scope of aux lock to the aux
> > transaction to aux done inside
> > the loop so we don't need to create this.
>
> Lock also needs to protect against concurrency changes in the aux ch
> control registers
I believe that spec page tells that we only need to protect from 6 to 9
So that would be included already inside the loop so we wouldn't even need
the retry for outside... although your approach on this part with broader
context and retry might be safer.
>
> >
> > > pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >
> > > if (vdd)
> > > @@ -1496,6 +1529,8 @@ 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);
> > > }
> > >
> > > static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 4b1d357a43ae..9fc54db3a79c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1040,6 +1040,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;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 2ef374f936b9..8b456e4e1b47 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > > EDP_PSR_DEBUG_MASK_HPD |
> > > EDP_PSR_DEBUG_MASK_LPSP);
> > > }
> > > +
> > > + if (intel_dp->aux_ch_mutex_reg.reg)
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> >
> > you don't need those here.
> > It is just needed on aux_transfer on the driver.
> > the psr aux transactions are done on our back by the display engine
> > so setting up this mutex here you block psr completely.
>
> this just enables to mutex, it is not getting the lock. From this point
> and on if hardware decides to send any aux transaction by it self it
> will get the lock avoiding any concurrency problems.
>
> I was thinking now in always have it enabled in skl+ and move it to
> intel_aux_reg_init() as getting the lock is also necessay to GTC(but we
> don't have this implemented yet), what do you think?
I think the enable *is* the lock get. I'm almost sure this approach will just hang
the display engine.
>
> >
> > > }
> > >
> > > /**
> > > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp
> > > *intel_dp,
> > > else
> > > WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > EDP_PSR_ENABLE);
> > > }
> > > +
> > > + if (intel_dp->aux_ch_mutex_reg.reg)
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > ~DP_AUX_CH_MUTEX_ENABLE);
> > > }
> > >
> > > /**
> > > --
> > > 2.16.1
> > >
> >
> > _______________________________________________
> > Intel-gfx-trybot mailing list
> > Intel-gfx-trybot at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx-trybot
More information about the Intel-gfx-trybot
mailing list