| Message ID | 20180227212359.15584-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/intel-gfx-prm-osrc-skl-vol12-display.pdf > Page 198 - AUX programming sequence > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@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_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; > + > + /* 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. > + > drm_dp_aux_init(&intel_dp->aux); > > /* Failure to allocate our preferred name is not critical */ > -- > 2.16.2
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/intel-gfx-prm-osrc-skl-vol12-display.pdf > > Page 198 - AUX programming sequence > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@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_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; > > + > > + /* 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. > The problem was with enabling the mutex without modifying the status bit. The earlier version > + ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp); > + I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE) had this in _trylock() This overwrites the status bit. > > + > > drm_dp_aux_init(&intel_dp->aux); > > > > /* Failure to allocate our preferred name is not critical */ > > -- > > 2.16.2 >
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/intel-g > > fx-prm-osrc-skl-vol12-display.pdf > > Page 198 - AUX programming sequence > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@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_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; > > + > > + /* 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? > > > + > > drm_dp_aux_init(&intel_dp->aux); > > > > /* Failure to allocate our preferred name is not critical > > */ > > -- > > 2.16.2 > >
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/intel-g > > > fx-prm-osrc-skl-vol12-display.pdf > > > Page 198 - AUX programming sequence > > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@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_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; > > > + > > > + /* 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?
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@intel.com > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@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. >
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@intel.com > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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.
On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote: > 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@intel.com > > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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. > +static bool intel_dp_aux_trylock(struct intel_dp *intel_dp) +{ + bool enabled; + u32 mutex; + + /* READ will try to lock the aux channel if the mutex was already + * enabled. + */ + mutex = I915_READ(DP_AUX_CH_MUTEX); + enabled = mutex & DP_AUX_CH_MUTEX_ENABLE; + + if (!enabled) { + /* mutex is currently disabled, enable it without altering the + * status bit in case the HW is using the bit */ + I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch), + mutex | DP_AUX_CH_MUTEX_ENABLE); + + /* Now that the mutex is enabled, request to lock it */ + mutex = I915_READ(DP_AUX_CH_MUTEX); + } + + if (mutex & DP_AUX_CH_MUTEX_STATUS == 0) + return true; + + 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; +} + Have I missed anything here?
Ville Syrjälä <ville.syrjala@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@intel.com >> > > > > > >> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> >> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > > Signed-off-by: José Roberto de Souza <jose.souza@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 02, 2018 at 11:20:42PM +0000, Pandiyan, Dhinakaran wrote: > > > > On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote: > > 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@intel.com > > > > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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. > > > > > +static bool intel_dp_aux_trylock(struct intel_dp *intel_dp) > +{ > + bool enabled; > + u32 mutex; > + > + /* READ will try to lock the aux channel if the mutex was > already > + * enabled. > + */ > + mutex = I915_READ(DP_AUX_CH_MUTEX); > + enabled = mutex & DP_AUX_CH_MUTEX_ENABLE; > + > + if (!enabled) { > + /* mutex is currently disabled, enable it without > altering the > + * status bit in case the HW is using the bit */ > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch), > + mutex | DP_AUX_CH_MUTEX_ENABLE); > + > + /* Now that the mutex is enabled, request to lock it */ > + mutex = I915_READ(DP_AUX_CH_MUTEX); > + } > + > + if (mutex & DP_AUX_CH_MUTEX_STATUS == 0) > + return true; > + > + 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; > +} > + > > > Have I missed anything here? ops, I wrote before seeing this here. I like this approach a lot, although I don't believe it is harmful to just always set enabled bit. anyways, +1 for this and probably only setting here. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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_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; + + /* 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); + drm_dp_aux_init(&intel_dp->aux); /* Failure to allocate our preferred name is not critical */