[Intel-gfx] [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
Rodrigo Vivi
rodrigo.vivi at gmail.com
Sat Mar 3 00:12:49 UTC 2018
Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
> On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
>> On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
>> > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
>> > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
>> > > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
>> > > > > el-g
>> > > > > fx-prm-osrc-skl-vol12-display.pdf
>> > > > > Page 198 - AUX programming sequence
>> > > > >
>> > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com
>> > > > > >
>> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> > > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>> > > > > ---
>> > > > >
>> > > > > Changelog:
>> > > > > v2
>> > > > > - removed the PSR dependency, now getting lock all the times
>> > > > > when
>> > > > > available
>> > > > > - renamed functions to avoid nested calls
>> > > > > - moved register bits right after the DP_AUX_CH_MUTEX()
>> > > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
>> > > > > Dhinakaran Pandiyan will sent a better and final version
>> > > > > v3
>> > > > > - rebased on top of Ville's AUX series
>> > > > > - moved port registers to above DP_AUX_CH_MUTEX()
>> > > > > - using intel_wait_for_register() instead of the internal
>> > > > > version
>> > > > > v4
>> > > > > - removed virtual function to get mutex register address
>> > > > > - enabling the mutex back only on aux channel init
>> > > > > - added the aux channel name to the timeout debug message
>> > > > > v5
>> > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
>> > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
>> > > > >
>> > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
>> > > > > drivers/gpu/drm/i915/intel_dp.c | 47
>> > > > > +++++++++++++++++++++++++++++++++++++++++
>> > > > > 2 files changed, 56 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > b/drivers/gpu/drm/i915/i915_reg.h
>> > > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
>> > > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > @@ -1081,6 +1081,42 @@ static uint32_t
>> > > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>> > > > > aux_clock_divi
>> > > > > der)
>> > > > > ;
>> > > > > }
>> > > > >
>> > > > > +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;
>> > > > > +
>> > > > > + /* Spec says that mutex is acquired when status bit is
>> > > > > read as unset,
>> > > > > + * here waiting for 2msec(+-4 aux transactions) before
>> > > > > give up.
>> > > > > + */
>> > > > > + if (intel_wait_for_register(dev_priv,
>> > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + DP_AUX_CH_MUTEX_STATUS, 0,
>> > > > > 2))
>> > > > > {
>> > > > > + DRM_DEBUG_KMS("aux channel %c locked for
>> > > > > 2msec,
>> > > > > timing out\n",
>> > > > > + aux_ch_name(intel_dp->aux_ch));
>> > > > > + 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;
>> > > > > +
>> > > > > + /* set the status bit releases the mutex + keeping
>> > > > > mutex
>> > > > > enabled */
>> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + 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,
>> > > > > @@ -1119,6 +1155,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_aux_ch_unlocked;
>> > > > > + }
>> > > > > +
>> > > > > /* Try to wait for any previous AUX channel activity
>> > > > > */
>> > > > > for (try = 0; try < 3; try++) {
>> > > > > status = I915_READ_NOTRACE(ch_ctl);
>> > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
>> > > > > *intel_dp,
>> > > > >
>> > > > > ret = recv_bytes;
>> > > > > out:
>> > > > > + intel_dp_aux_ch_unlock(intel_dp);
>> > > > > +out_aux_ch_unlocked:
>> > > > > pm_qos_update_request(&dev_priv->pm_qos,
>> > > > > PM_QOS_DEFAULT_VALUE);
>> > > > >
>> > > > > if (vdd)
>> > > > > @@ -1536,6 +1579,10 @@ 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)
>> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + DP_AUX_CH_MUTEX_ENABLE);
>> > > >
>> > > > And who enables it after system/runtime PM etc.? Not sure how
>> > > > much
>> > > > sense
>> > > > there is to finesse this anyway. One extra mmio write every time
>> > > > we
>> > > > try
>> > > > to acquire the mutex shouldn't kill anyone.
>> > >
>> > > Valid point, but I guess is better enable the mutex in
>> > > intel_dp_encoder_reset():
>> > >
>> > > "
>> > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
>> > > drm_encoder
>> > > *encoder)
>> > >
>> > > pps_lock(intel_dp);
>> > >
>> > > + if (INTEL_GEN(dev_priv) >= 9)
>> > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > + DP_AUX_CH_MUTEX_ENABLE);
>> > > +
>> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
>> > > "
>> > >
>> > > any objections?
>> >
>> > I guess we can hope that dmc takes care of it for dc5/6. But what
>> > about dc9?
>>
>> Yep, for DC5 and DC6 the registers are restored by hardware but it is
>> not the case for DC9. If I'm not missing something looks like there is
>> no handling to restore registers when exiting DC9.
>
> The design of the driver is "program all required registers when you
> need them" instead of the "blindly save/restore everything" DMC firmware
> approach.
So, what do you suggest Ville?
Some function called form i915_drm_resume that loops on connectors
with active DP and enable its aux mutex?
But even on this case we would have the risk of unsetting the status
while hw can be using, no?
Or this or:
Option 2: Enable without RMW only on trylock and disabe without RMW on release.
Or (my favorite one) that follows spec line by line:
Option 3: Enable with RMW only on trylock and with RMW touch only Status
bit keeping it enabled.
I won't believe RMW would have any impact here because on trylock we are
anyways trying to lock so reading the status right after enabling.
Also there is no problem on RMW on release because right after reading
before writing we would be releasing it anyways.
So RMW is the simplified solution imho...
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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