[Intel-gfx] [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
Pandiyan, Dhinakaran
dhinakaran.pandiyan at intel.com
Thu Feb 22 20:25:46 UTC 2018
On Thu, 2018-02-22 at 12:13 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> > On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä 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-gfx-prm-osrc-skl-vol12-display.pdf
> > > > > Page 198 - AUX programming sequence
> > > >
> > > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > > around aux ch usage on those platforms. Not sure how big of a locking
> > > > inversion exercise it would be to make that happen though.
> > >
> > > True. But I believe that can be handled differently.
> > > Probably with Aux power domains blocking PSR off "domain".
> > > Something similar to DC off.
> > >
> > > I actually had patches for that lost somewhere... :(
> > >
> >
> > Just a note, the spec explicitly says we should disable PSR around AUX
> > transactions for BDW (and HSW iirc).
> >
> > About using the power domain infra, I am not quite sure. The _get() and
> > _put() idiom makes sense when we want reference counting to handle
> > multiple consumers that want PSR to be disabled. Does anything else need
> > to enable/disable PSR?
>
> VLV/CHV vblanks?! :/
>
I keep forgetting VLV/CHV, sorry. This brings us back to the whole
"calling blocking functions from atomic contexts" problem. Not sure how
to handle that.
Assuming we solve that problem, we should also probably avoid polluting
the power domain code and add reference counting for PSR. However, since
all this is not required for BDW/HSW, I am inclined towards a simpler
solution for BDW/HSW AUX usage.
> >
> >
> > > >
> > > > >
> > > > > 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)
> > > > >
> > > > > +#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);
> > > > > +
> > > > > + /* 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)) {
> > > > > + 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);
> > > > > +}
> > > > > +
> > > > > 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;
> > > > > + }
> > > > > +
> > > > > /* 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);
> > > > > + }
> > > > > }
> > > > >
> > > > > 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
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list