While enabling/disabling DPMS before link training with MST hubs is
perfectly valid; unfortunately disabling DPMS results in some devices
disabling their AUX CH block as well. For SST this isn't as much of a
problem, but for MST we need to be able to continue handling aux
transactions even when none of the sinks are turned on since it's
possible for us to have a single atomic commit which results in
disabling each downstream sink, followed by subsequently re-enabling
each sink.
If we don't do this, we'll end up stalling any pending ESI interrupts
from the sink for up to 1ms. Unfortunately, dropping ESIs during this
timespan makes it so that link fallback retraining for MST (which I will
be submitting to the ML shortly) fails due to the channel EQ failure
interrupts potentially getting dropped. Additionally, when performing a
modeset that brings the hub status's link status from bad -> good having
ESIs disabled for that long causes us to miss the hub's response to us
trying to start link training as well.
Since any sink with MST is going to support DisplayPort 1.2 anyway, save
us the hassle of trying to wait until the sink comes back up and just
never shut the aux block down.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Dhinakaran Pandiyan <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: [email protected]
Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
---
drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..0479c377981b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
return;
if (mode != DRM_MODE_DPMS_ON) {
+ unsigned char data = intel_dp->is_mst ?
+ DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
+
if (downstream_hpd_needs_d0(intel_dp))
return;
- ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
- DP_SET_POWER_D3);
+ ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
} else {
struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
--
2.14.3
While enabling/disabling DPMS before link training with MST hubs is
perfectly valid; unfortunately disabling DPMS results in some devices
disabling their AUX CH block as well. For SST this isn't as much of a
problem, but for MST we need to be able to continue handling aux
transactions even when none of the sinks are turned on since it's
possible for us to have a single atomic commit which results in
disabling each downstream sink, followed by subsequently re-enabling
each sink.
If we don't do this, we'll end up stalling any pending ESI interrupts
from the sink for up to 1ms. Unfortunately, dropping ESIs during this
timespan makes it so that link fallback retraining for MST (which I will
be submitting to the ML shortly) fails due to the channel EQ failure
interrupts potentially getting dropped. Additionally, when performing a
modeset that brings the hub status's link status from bad -> good having
ESIs disabled for that long causes us to miss the hub's response to us
trying to start link training as well.
Since any sink with MST is going to support DisplayPort 1.2 anyway, save
us the hassle of trying to wait until the sink comes back up and just
never shut the aux block down.
Changes since v2:
- Fix patch name, no functional changes
Signed-off-by: Lyude Paul <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Dhinakaran Pandiyan <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: [email protected]
Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
---
drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..0479c377981b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
return;
if (mode != DRM_MODE_DPMS_ON) {
+ unsigned char data = intel_dp->is_mst ?
+ DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
+
if (downstream_hpd_needs_d0(intel_dp))
return;
- ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
- DP_SET_POWER_D3);
+ ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
} else {
struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
--
2.14.3
On Mon, 2018-04-02 at 17:21 -0400, Lyude Paul wrote:
> While enabling/disabling DPMS before link training with MST hubs is
> perfectly valid; unfortunately disabling DPMS results in some devices
> disabling their AUX CH block as well. For SST this isn't as much of a
> problem, but for MST we need to be able to continue handling aux
> transactions even when none of the sinks are turned on since it's
> possible for us to have a single atomic commit which results in
> disabling each downstream sink, followed by subsequently re-enabling
> each sink.
>
> If we don't do this, we'll end up stalling any pending ESI interrupts
> from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> timespan makes it so that link fallback retraining for MST (which I will
> be submitting to the ML shortly) fails due to the channel EQ failure
> interrupts potentially getting dropped. Additionally, when performing a
> modeset that brings the hub status's link status from bad -> good having
> ESIs disabled for that long causes us to miss the hub's response to us
> trying to start link training as well.
>
> Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> us the hassle of trying to wait until the sink comes back up and just
> never shut the aux block down.
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Dhinakaran Pandiyan <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Cc: [email protected]
> Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
We've come a full circle on this one :)
1) Originally, we had disable_ddi() setting the branch power state to
D3.
2) Then "Use MST sideband message transactions for dpms control" removed
that as a bug fix for some devices. The sideband solution was chosen
over the D3_AUX_ON approach this patch takes.
3) Next, "Write to SET_POWER dpcd to enable MST hub" effectively took us
back to the original state due to a regression.
4) Now, we are calling (3) a regression and implementing D3_AUX_ON.
I guess, this combination should work for most devices. But, we should
test this on a few different MST hubs before going ahead.
-DK
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..0479c377981b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> return;
>
> if (mode != DRM_MODE_DPMS_ON) {
> + unsigned char data = intel_dp->is_mst ?
> + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> +
> if (downstream_hpd_needs_d0(intel_dp))
> return;
>
> - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> - DP_SET_POWER_D3);
> + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
> } else {
> struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>
On 04/02/2018 02:26 PM, Lyude Paul wrote:
> While enabling/disabling DPMS before link training with MST hubs is
> perfectly valid; unfortunately disabling DPMS results in some devices
> disabling their AUX CH block as well. For SST this isn't as much of a
> problem, but for MST we need to be able to continue handling aux
> transactions even when none of the sinks are turned on since it's
> possible for us to have a single atomic commit which results in
> disabling each downstream sink, followed by subsequently re-enabling
> each sink.
>
> If we don't do this, we'll end up stalling any pending ESI interrupts
> from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> timespan makes it so that link fallback retraining for MST (which I will
> be submitting to the ML shortly) fails due to the channel EQ failure
> interrupts potentially getting dropped. Additionally, when performing a
> modeset that brings the hub status's link status from bad -> good having
> ESIs disabled for that long causes us to miss the hub's response to us
> trying to start link training as well.
>
> Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> us the hassle of trying to wait until the sink comes back up and just
> never shut the aux block down.
>
> Changes since v2:
> - Fix patch name, no functional changes
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Dhinakaran Pandiyan <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Cc: [email protected]
> Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
Still able to boot docked and lid closed so
Tested-by: Laura Abbott <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..0479c377981b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> return;
>
> if (mode != DRM_MODE_DPMS_ON) {
> + unsigned char data = intel_dp->is_mst ?
> + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> +
> if (downstream_hpd_needs_d0(intel_dp))
> return;
>
> - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> - DP_SET_POWER_D3);
> + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
> } else {
> struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>
>
On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> While enabling/disabling DPMS before link training with MST hubs is
> perfectly valid; unfortunately disabling DPMS results in some devices
> disabling their AUX CH block as well. For SST this isn't as much of a
> problem, but for MST we need to be able to continue handling aux
> transactions even when none of the sinks are turned on since it's
> possible for us to have a single atomic commit which results in
> disabling each downstream sink, followed by subsequently re-enabling
> each sink.
>
> If we don't do this, we'll end up stalling any pending ESI interrupts
> from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> timespan makes it so that link fallback retraining for MST (which I will
> be submitting to the ML shortly) fails due to the channel EQ failure
> interrupts potentially getting dropped. Additionally, when performing a
> modeset that brings the hub status's link status from bad -> good having
> ESIs disabled for that long causes us to miss the hub's response to us
> trying to start link training as well.
>
> Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> us the hassle of trying to wait until the sink comes back up and just
> never shut the aux block down.
>
> Changes since v2:
> - Fix patch name, no functional changes
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Dhinakaran Pandiyan <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Cc: [email protected]
> Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..0479c377981b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> return;
>
> if (mode != DRM_MODE_DPMS_ON) {
> + unsigned char data = intel_dp->is_mst ?
> + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
This smells like a workaround for an actual bug somewhere. Why exactly
is the slower wakeup or the AUX block a problem for MST but not for SST
when the link training is exactly the same for SST and MST?
> +
> if (downstream_hpd_needs_d0(intel_dp))
> return;
>
> - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> - DP_SET_POWER_D3);
> + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
> } else {
> struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>
> --
> 2.14.3
--
Ville Syrj?l?
Intel OTC
On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
> On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > While enabling/disabling DPMS before link training with MST hubs is
> > perfectly valid; unfortunately disabling DPMS results in some devices
> > disabling their AUX CH block as well. For SST this isn't as much of a
> > problem, but for MST we need to be able to continue handling aux
> > transactions even when none of the sinks are turned on since it's
> > possible for us to have a single atomic commit which results in
> > disabling each downstream sink, followed by subsequently re-enabling
> > each sink.
> >
> > If we don't do this, we'll end up stalling any pending ESI interrupts
> > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > timespan makes it so that link fallback retraining for MST (which I will
> > be submitting to the ML shortly) fails due to the channel EQ failure
> > interrupts potentially getting dropped. Additionally, when performing a
> > modeset that brings the hub status's link status from bad -> good having
> > ESIs disabled for that long causes us to miss the hub's response to us
> > trying to start link training as well.
> >
> > Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> > us the hassle of trying to wait until the sink comes back up and just
> > never shut the aux block down.
> >
> > Changes since v2:
> > - Fix patch name, no functional changes
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Cc: Laura Abbott <[email protected]>
> > Cc: Dhinakaran Pandiyan <[email protected]>
> > Cc: Ville Syrjälä <[email protected]>
> > Cc: [email protected]
> > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST
> > hub.")
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..0479c377981b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp,
> > int mode)
> > return;
> >
> > if (mode != DRM_MODE_DPMS_ON) {
> > + unsigned char data = intel_dp->is_mst ?
> > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
>
> This smells like a workaround for an actual bug somewhere. Why exactly
> is the slower wakeup or the AUX block a problem for MST but not for SST
> when the link training is exactly the same for SST and MST?
I actually thought about this but I still think this is the appropriate fix.
So; the real reason for the wakeup not being a problem with SST is that for
DPMS on with SST, we actually do a wait to make sure that the hub is ready
before continuing. And yes: I'm fairly sure SST does actually have around the
same wakeup time that MST does, but with the wait we do it doesn't reallhy
make a difference. With MST, we could do this but there's a few reasons I
don't think we should:
* We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that has
MST is going to be guaranteed to have this.
* Turning off the aux block means that there's a high chance we're going to
miss ESIs from sinks
* It's faster to keep the aux block on anyway
>
> > +
> > if (downstream_hpd_needs_d0(intel_dp))
> > return;
> >
> > - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > - DP_SET_POWER_D3);
> > + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > data);
> > } else {
> > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> >
> > --
> > 2.14.3
>
>
--
Cheers,
Lyude Paul
On Wed, Apr 04, 2018 at 06:34:29PM +0300, Ville Syrj?l? wrote:
> On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > While enabling/disabling DPMS before link training with MST hubs is
> > perfectly valid; unfortunately disabling DPMS results in some devices
> > disabling their AUX CH block as well. For SST this isn't as much of a
> > problem, but for MST we need to be able to continue handling aux
> > transactions even when none of the sinks are turned on since it's
> > possible for us to have a single atomic commit which results in
> > disabling each downstream sink, followed by subsequently re-enabling
> > each sink.
> >
> > If we don't do this, we'll end up stalling any pending ESI interrupts
> > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > timespan makes it so that link fallback retraining for MST (which I will
> > be submitting to the ML shortly) fails due to the channel EQ failure
> > interrupts potentially getting dropped. Additionally, when performing a
> > modeset that brings the hub status's link status from bad -> good having
> > ESIs disabled for that long causes us to miss the hub's response to us
> > trying to start link training as well.
> >
> > Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> > us the hassle of trying to wait until the sink comes back up and just
> > never shut the aux block down.
> >
> > Changes since v2:
> > - Fix patch name, no functional changes
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Cc: Laura Abbott <[email protected]>
> > Cc: Dhinakaran Pandiyan <[email protected]>
> > Cc: Ville Syrj?l? <[email protected]>
> > Cc: [email protected]
> > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4298ac..0479c377981b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > return;
> >
> > if (mode != DRM_MODE_DPMS_ON) {
> > + unsigned char data = intel_dp->is_mst ?
> > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
>
> This smells like a workaround for an actual bug somewhere. Why exactly
> is the slower wakeup or the AUX block a problem for MST but not for SST
> when the link training is exactly the same for SST and MST?
>
The problem occurs only in case of MST because the Channel EQ failure is notified
through ESI sideband AUX messages which potentially can get dropped because of disabling
DPMS. But in case of SST, we detect the channel EQ failure write during the EQ TPS sequence
which happens on the main link.
Manasi
> > +
> > if (downstream_hpd_needs_d0(intel_dp))
> > return;
> >
> > - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > - DP_SET_POWER_D3);
> > + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, data);
> > } else {
> > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> >
> > --
> > 2.14.3
>
> --
> Ville Syrj?l?
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2018-04-04 at 11:44 -0700, Manasi Navare wrote:
> On Wed, Apr 04, 2018 at 06:34:29PM +0300, Ville Syrjälä wrote:
> > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > While enabling/disabling DPMS before link training with MST hubs is
> > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > problem, but for MST we need to be able to continue handling aux
> > > transactions even when none of the sinks are turned on since it's
> > > possible for us to have a single atomic commit which results in
> > > disabling each downstream sink, followed by subsequently re-enabling
> > > each sink.
> > >
> > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > timespan makes it so that link fallback retraining for MST (which I will
> > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > interrupts potentially getting dropped. Additionally, when performing a
> > > modeset that brings the hub status's link status from bad -> good having
> > > ESIs disabled for that long causes us to miss the hub's response to us
> > > trying to start link training as well.
> > >
> > > Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> > > us the hassle of trying to wait until the sink comes back up and just
> > > never shut the aux block down.
> > >
> > > Changes since v2:
> > > - Fix patch name, no functional changes
> > >
> > > Signed-off-by: Lyude Paul <[email protected]>
> > > Cc: Laura Abbott <[email protected]>
> > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > Cc: Ville Syrjälä <[email protected]>
> > > Cc: [email protected]
> > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable
> > > MST hub.")
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 62f82c4298ac..0479c377981b 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > *intel_dp, int mode)
> > > return;
> > >
> > > if (mode != DRM_MODE_DPMS_ON) {
> > > + unsigned char data = intel_dp->is_mst ?
> > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> >
> > This smells like a workaround for an actual bug somewhere. Why exactly
> > is the slower wakeup or the AUX block a problem for MST but not for SST
> > when the link training is exactly the same for SST and MST?
> >
>
> The problem occurs only in case of MST because the Channel EQ failure is
> notified
> through ESI sideband AUX messages which potentially can get dropped because
> of disabling
> DPMS. But in case of SST, we detect the channel EQ failure write during the
> EQ TPS sequence
> which happens on the main link.
mhm- that is the big problem it causes, at least with this patchset. I've been
considering maybe looking into probing downstream sinks with remote dpcd reads
to see their link training status, as I think that might actually be the real
reason for why there's this weird difference between the channel eq status in
the esi and the actual link training status of the hub in the dpcd registers
that are shared with SST. but that's for a later date :)
>
> Manasi
>
> > > +
> > > if (downstream_hpd_needs_d0(intel_dp))
> > > return;
> > >
> > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > - DP_SET_POWER_D3);
> > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > data);
> > > } else {
> > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > >
> > > --
> > > 2.14.3
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Cheers,
Lyude Paul
On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> On Wed, 2018-04-04 at 18:34 +0300, Ville Syrj?l? wrote:
> > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > While enabling/disabling DPMS before link training with MST hubs is
> > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > problem, but for MST we need to be able to continue handling aux
> > > transactions even when none of the sinks are turned on since it's
> > > possible for us to have a single atomic commit which results in
> > > disabling each downstream sink, followed by subsequently re-enabling
> > > each sink.
> > >
> > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > timespan makes it so that link fallback retraining for MST (which I will
> > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > interrupts potentially getting dropped. Additionally, when performing a
> > > modeset that brings the hub status's link status from bad -> good having
> > > ESIs disabled for that long causes us to miss the hub's response to us
> > > trying to start link training as well.
> > >
> > > Since any sink with MST is going to support DisplayPort 1.2 anyway, save
> > > us the hassle of trying to wait until the sink comes back up and just
> > > never shut the aux block down.
> > >
> > > Changes since v2:
> > > - Fix patch name, no functional changes
> > >
> > > Signed-off-by: Lyude Paul <[email protected]>
> > > Cc: Laura Abbott <[email protected]>
> > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > Cc: Ville Syrj?l? <[email protected]>
> > > Cc: [email protected]
> > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST
> > > hub.")
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 62f82c4298ac..0479c377981b 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp,
> > > int mode)
> > > return;
> > >
> > > if (mode != DRM_MODE_DPMS_ON) {
> > > + unsigned char data = intel_dp->is_mst ?
> > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> >
> > This smells like a workaround for an actual bug somewhere. Why exactly
> > is the slower wakeup or the AUX block a problem for MST but not for SST
> > when the link training is exactly the same for SST and MST?
> I actually thought about this but I still think this is the appropriate fix.
> So; the real reason for the wakeup not being a problem with SST is that for
> DPMS on with SST, we actually do a wait to make sure that the hub is ready
> before continuing. And yes: I'm fairly sure SST does actually have around the
> same wakeup time that MST does, but with the wait we do it doesn't reallhy
> make a difference. With MST, we could do this but there's a few reasons I
> don't think we should:
> * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that has
> MST is going to be guaranteed to have this.
> * Turning off the aux block means that there's a high chance we're going to
> miss ESIs from sinks
And how exactly do we lose irqs? The hub/whatever throws the up req msgs
away if we don't read them within some really short time?
> * It's faster to keep the aux block on anyway
>
>
> >
> > > +
> > > if (downstream_hpd_needs_d0(intel_dp))
> > > return;
> > >
> > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > - DP_SET_POWER_D3);
> > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > data);
> > > } else {
> > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > >
> > > --
> > > 2.14.3
> >
> >
> --
> Cheers,
> Lyude Paul
--
Ville Syrj?l?
Intel OTC
On Wed, 2018-04-04 at 21:53 +0300, Ville Syrjälä wrote:
> On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
> > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > While enabling/disabling DPMS before link training with MST hubs is
> > > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > > problem, but for MST we need to be able to continue handling aux
> > > > transactions even when none of the sinks are turned on since it's
> > > > possible for us to have a single atomic commit which results in
> > > > disabling each downstream sink, followed by subsequently re-enabling
> > > > each sink.
> > > >
> > > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > > timespan makes it so that link fallback retraining for MST (which I
> > > > will
> > > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > > interrupts potentially getting dropped. Additionally, when performing
> > > > a
> > > > modeset that brings the hub status's link status from bad -> good
> > > > having
> > > > ESIs disabled for that long causes us to miss the hub's response to us
> > > > trying to start link training as well.
> > > >
> > > > Since any sink with MST is going to support DisplayPort 1.2 anyway,
> > > > save
> > > > us the hassle of trying to wait until the sink comes back up and just
> > > > never shut the aux block down.
> > > >
> > > > Changes since v2:
> > > > - Fix patch name, no functional changes
> > > >
> > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > Cc: Laura Abbott <[email protected]>
> > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > Cc: Ville Syrjälä <[email protected]>
> > > > Cc: [email protected]
> > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable
> > > > MST
> > > > hub.")
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 62f82c4298ac..0479c377981b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > *intel_dp,
> > > > int mode)
> > > > return;
> > > >
> > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > + unsigned char data = intel_dp->is_mst ?
> > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > >
> > > This smells like a workaround for an actual bug somewhere. Why exactly
> > > is the slower wakeup or the AUX block a problem for MST but not for SST
> > > when the link training is exactly the same for SST and MST?
> >
> > I actually thought about this but I still think this is the appropriate
> > fix.
> > So; the real reason for the wakeup not being a problem with SST is that
> > for
> > DPMS on with SST, we actually do a wait to make sure that the hub is ready
> > before continuing. And yes: I'm fairly sure SST does actually have around
> > the
> > same wakeup time that MST does, but with the wait we do it doesn't reallhy
> > make a difference. With MST, we could do this but there's a few reasons I
> > don't think we should:
> > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that
> > has
> > MST is going to be guaranteed to have this.
> > * Turning off the aux block means that there's a high chance we're going
> > to
> > miss ESIs from sinks
>
> And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> away if we don't read them within some really short time?
That's my hypothesis at least. I'm betting that on the fact that when I was
implementing MST retraining before we put the intel_dp_check_mst_status() (or
whatever it's called) into the dig workqueue, getting the sink to go down and
come back up was a lot more unreliable whenever I introduced anything that
would block the esi handler for longer then a very brief period of time (say,
50-100ms?). I've seen some notes elsewhere too that seemed to imply for SST,
things were pretty sensitive to irq latency (line 1050, intel_dp.c) so it
wouldn't be terribly surprising if it's the same for MST. At the very least,
now that we've got the ESI handler running in the dig worker things seem to
have gotten a /lot/ more reliable now that we can basically go the whole
modeset without blocking the ESI handler for very long.
>
> > * It's faster to keep the aux block on anyway
> >
> >
> > >
> > > > +
> > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > return;
> > > >
> > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_SET_POWER,
> > > > - DP_SET_POWER_D3);
> > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_SET_POWER,
> > > > data);
> > > > } else {
> > > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > > >
> > > > --
> > > > 2.14.3
> > >
> > >
> >
> > --
> > Cheers,
> > Lyude Paul
>
>
--
Cheers,
Lyude Paul
On Wed, 2018-04-04 at 21:53 +0300, Ville Syrjälä wrote:
> On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
> > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > While enabling/disabling DPMS before link training with MST hubs is
> > > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > > problem, but for MST we need to be able to continue handling aux
> > > > transactions even when none of the sinks are turned on since it's
> > > > possible for us to have a single atomic commit which results in
> > > > disabling each downstream sink, followed by subsequently re-enabling
> > > > each sink.
> > > >
> > > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > > timespan makes it so that link fallback retraining for MST (which I
> > > > will
> > > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > > interrupts potentially getting dropped. Additionally, when performing
> > > > a
> > > > modeset that brings the hub status's link status from bad -> good
> > > > having
> > > > ESIs disabled for that long causes us to miss the hub's response to us
> > > > trying to start link training as well.
> > > >
> > > > Since any sink with MST is going to support DisplayPort 1.2 anyway,
> > > > save
> > > > us the hassle of trying to wait until the sink comes back up and just
> > > > never shut the aux block down.
> > > >
> > > > Changes since v2:
> > > > - Fix patch name, no functional changes
> > > >
> > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > Cc: Laura Abbott <[email protected]>
> > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > Cc: Ville Syrjälä <[email protected]>
> > > > Cc: [email protected]
> > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable
> > > > MST
> > > > hub.")
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 62f82c4298ac..0479c377981b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > *intel_dp,
> > > > int mode)
> > > > return;
> > > >
> > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > + unsigned char data = intel_dp->is_mst ?
> > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > >
> > > This smells like a workaround for an actual bug somewhere. Why exactly
> > > is the slower wakeup or the AUX block a problem for MST but not for SST
> > > when the link training is exactly the same for SST and MST?
> >
> > I actually thought about this but I still think this is the appropriate
> > fix.
> > So; the real reason for the wakeup not being a problem with SST is that
> > for
> > DPMS on with SST, we actually do a wait to make sure that the hub is ready
> > before continuing. And yes: I'm fairly sure SST does actually have around
> > the
> > same wakeup time that MST does, but with the wait we do it doesn't reallhy
> > make a difference. With MST, we could do this but there's a few reasons I
> > don't think we should:
> > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that
> > has
> > MST is going to be guaranteed to have this.
> > * Turning off the aux block means that there's a high chance we're going
> > to
> > miss ESIs from sinks
>
> And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> away if we don't read them within some really short time?
Oh-additionally I did forget to mention that i have actually witnessed the
channel eq failures in the ESI getting dropped without this patch. Meaning if
we miss them, there's a chance the hub may just not choose to send them again
for whatever reason.
>
> > * It's faster to keep the aux block on anyway
> >
> >
> > >
> > > > +
> > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > return;
> > > >
> > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_SET_POWER,
> > > > - DP_SET_POWER_D3);
> > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > DP_SET_POWER,
> > > > data);
> > > > } else {
> > > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > > >
> > > > --
> > > > 2.14.3
> > >
> > >
> >
> > --
> > Cheers,
> > Lyude Paul
>
>
--
Cheers,
Lyude Paul
On Wed, Apr 04, 2018 at 02:59:09PM -0400, Lyude Paul wrote:
> On Wed, 2018-04-04 at 21:53 +0300, Ville Syrj?l? wrote:
> > On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrj?l? wrote:
> > > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > > While enabling/disabling DPMS before link training with MST hubs is
> > > > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > > > problem, but for MST we need to be able to continue handling aux
> > > > > transactions even when none of the sinks are turned on since it's
> > > > > possible for us to have a single atomic commit which results in
> > > > > disabling each downstream sink, followed by subsequently re-enabling
> > > > > each sink.
> > > > >
> > > > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > > > timespan makes it so that link fallback retraining for MST (which I
> > > > > will
> > > > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > > > interrupts potentially getting dropped. Additionally, when performing
> > > > > a
> > > > > modeset that brings the hub status's link status from bad -> good
> > > > > having
> > > > > ESIs disabled for that long causes us to miss the hub's response to us
> > > > > trying to start link training as well.
> > > > >
> > > > > Since any sink with MST is going to support DisplayPort 1.2 anyway,
> > > > > save
> > > > > us the hassle of trying to wait until the sink comes back up and just
> > > > > never shut the aux block down.
> > > > >
> > > > > Changes since v2:
> > > > > - Fix patch name, no functional changes
> > > > >
> > > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > > Cc: Laura Abbott <[email protected]>
> > > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > > Cc: Ville Syrj?l? <[email protected]>
> > > > > Cc: [email protected]
> > > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable
> > > > > MST
> > > > > hub.")
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 62f82c4298ac..0479c377981b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > > *intel_dp,
> > > > > int mode)
> > > > > return;
> > > > >
> > > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > > + unsigned char data = intel_dp->is_mst ?
> > > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > > >
> > > > This smells like a workaround for an actual bug somewhere. Why exactly
> > > > is the slower wakeup or the AUX block a problem for MST but not for SST
> > > > when the link training is exactly the same for SST and MST?
> > >
> > > I actually thought about this but I still think this is the appropriate
> > > fix.
> > > So; the real reason for the wakeup not being a problem with SST is that
> > > for
> > > DPMS on with SST, we actually do a wait to make sure that the hub is ready
> > > before continuing. And yes: I'm fairly sure SST does actually have around
> > > the
> > > same wakeup time that MST does, but with the wait we do it doesn't reallhy
> > > make a difference. With MST, we could do this but there's a few reasons I
> > > don't think we should:
> > > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that
> > > has
> > > MST is going to be guaranteed to have this.
> > > * Turning off the aux block means that there's a high chance we're going
> > > to
> > > miss ESIs from sinks
> >
> > And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> > away if we don't read them within some really short time?
> That's my hypothesis at least. I'm betting that on the fact that when I was
> implementing MST retraining before we put the intel_dp_check_mst_status() (or
> whatever it's called) into the dig workqueue, getting the sink to go down and
> come back up was a lot more unreliable whenever I introduced anything that
> would block the esi handler for longer then a very brief period of time (say,
> 50-100ms?). I've seen some notes elsewhere too that seemed to imply for SST,
> things were pretty sensitive to irq latency (line 1050, intel_dp.c) so it
> wouldn't be terribly surprising if it's the same for MST. At the very least,
> now that we've got the ESI handler running in the dig worker things seem to
> have gotten a /lot/ more reliable now that we can basically go the whole
> modeset without blocking the ESI handler for very long.
Hmm. OK, so the spec seems to be saying that we have 100ms to read
the UP_REQ/DOWN_REPLY msg after the IRQ_HPD. That's still a lot more
than the 1ms max allowed wakeup time. Looks like there's a extended
wakeup time request/grant mechanism now, but without the explicit
grant (which we don't do) the 1ms still holds.
> >
> > > * It's faster to keep the aux block on anyway
> > >
> > >
> > > >
> > > > > +
> > > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > > return;
> > > > >
> > > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > DP_SET_POWER,
> > > > > - DP_SET_POWER_D3);
> > > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > DP_SET_POWER,
> > > > > data);
> > > > > } else {
> > > > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > > > >
> > > > > --
> > > > > 2.14.3
> > > >
> > > >
> > >
> > > --
> > > Cheers,
> > > Lyude Paul
> >
> >
> --
> Cheers,
> Lyude Paul
--
Ville Syrj?l?
Intel OTC
On Wed, Apr 04, 2018 at 03:00:12PM -0400, Lyude Paul wrote:
> On Wed, 2018-04-04 at 21:53 +0300, Ville Syrj?l? wrote:
> > On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrj?l? wrote:
> > > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > > While enabling/disabling DPMS before link training with MST hubs is
> > > > > perfectly valid; unfortunately disabling DPMS results in some devices
> > > > > disabling their AUX CH block as well. For SST this isn't as much of a
> > > > > problem, but for MST we need to be able to continue handling aux
> > > > > transactions even when none of the sinks are turned on since it's
> > > > > possible for us to have a single atomic commit which results in
> > > > > disabling each downstream sink, followed by subsequently re-enabling
> > > > > each sink.
> > > > >
> > > > > If we don't do this, we'll end up stalling any pending ESI interrupts
> > > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during this
> > > > > timespan makes it so that link fallback retraining for MST (which I
> > > > > will
> > > > > be submitting to the ML shortly) fails due to the channel EQ failure
> > > > > interrupts potentially getting dropped. Additionally, when performing
> > > > > a
> > > > > modeset that brings the hub status's link status from bad -> good
> > > > > having
> > > > > ESIs disabled for that long causes us to miss the hub's response to us
> > > > > trying to start link training as well.
> > > > >
> > > > > Since any sink with MST is going to support DisplayPort 1.2 anyway,
> > > > > save
> > > > > us the hassle of trying to wait until the sink comes back up and just
> > > > > never shut the aux block down.
> > > > >
> > > > > Changes since v2:
> > > > > - Fix patch name, no functional changes
> > > > >
> > > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > > Cc: Laura Abbott <[email protected]>
> > > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > > Cc: Ville Syrj?l? <[email protected]>
> > > > > Cc: [email protected]
> > > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable
> > > > > MST
> > > > > hub.")
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 62f82c4298ac..0479c377981b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > > *intel_dp,
> > > > > int mode)
> > > > > return;
> > > > >
> > > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > > + unsigned char data = intel_dp->is_mst ?
> > > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > > >
> > > > This smells like a workaround for an actual bug somewhere. Why exactly
> > > > is the slower wakeup or the AUX block a problem for MST but not for SST
> > > > when the link training is exactly the same for SST and MST?
> > >
> > > I actually thought about this but I still think this is the appropriate
> > > fix.
> > > So; the real reason for the wakeup not being a problem with SST is that
> > > for
> > > DPMS on with SST, we actually do a wait to make sure that the hub is ready
> > > before continuing. And yes: I'm fairly sure SST does actually have around
> > > the
> > > same wakeup time that MST does, but with the wait we do it doesn't reallhy
> > > make a difference. With MST, we could do this but there's a few reasons I
> > > don't think we should:
> > > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that
> > > has
> > > MST is going to be guaranteed to have this.
> > > * Turning off the aux block means that there's a high chance we're going
> > > to
> > > miss ESIs from sinks
> >
> > And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> > away if we don't read them within some really short time?
> Oh-additionally I did forget to mention that i have actually witnessed the
> channel eq failures in the ESI getting dropped without this patch.
Not sure what that means. I don't think there is any sideband messaging
involved in link training so not sure what is dropped in this case. The
link status/etc. are just polled directly by the upstream device.
> Meaning if
> we miss them, there's a chance the hub may just not choose to send them again
> for whatever reason.
> >
> > > * It's faster to keep the aux block on anyway
> > >
> > >
> > > >
> > > > > +
> > > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > > return;
> > > > >
> > > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > DP_SET_POWER,
> > > > > - DP_SET_POWER_D3);
> > > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > DP_SET_POWER,
> > > > > data);
> > > > > } else {
> > > > > struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > > > >
> > > > > --
> > > > > 2.14.3
> > > >
> > > >
> > >
> > > --
> > > Cheers,
> > > Lyude Paul
> >
> >
> --
> Cheers,
> Lyude Paul
--
Ville Syrj?l?
Intel OTC
On Wed, 2018-04-04 at 22:31 +0300, Ville Syrjälä wrote:
> On Wed, Apr 04, 2018 at 02:59:09PM -0400, Lyude Paul wrote:
> > On Wed, 2018-04-04 at 21:53 +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > > > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
> > > > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > > > While enabling/disabling DPMS before link training with MST hubs
> > > > > > is
> > > > > > perfectly valid; unfortunately disabling DPMS results in some
> > > > > > devices
> > > > > > disabling their AUX CH block as well. For SST this isn't as much
> > > > > > of a
> > > > > > problem, but for MST we need to be able to continue handling aux
> > > > > > transactions even when none of the sinks are turned on since it's
> > > > > > possible for us to have a single atomic commit which results in
> > > > > > disabling each downstream sink, followed by subsequently re-
> > > > > > enabling
> > > > > > each sink.
> > > > > >
> > > > > > If we don't do this, we'll end up stalling any pending ESI
> > > > > > interrupts
> > > > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during
> > > > > > this
> > > > > > timespan makes it so that link fallback retraining for MST (which
> > > > > > I
> > > > > > will
> > > > > > be submitting to the ML shortly) fails due to the channel EQ
> > > > > > failure
> > > > > > interrupts potentially getting dropped. Additionally, when
> > > > > > performing
> > > > > > a
> > > > > > modeset that brings the hub status's link status from bad -> good
> > > > > > having
> > > > > > ESIs disabled for that long causes us to miss the hub's response
> > > > > > to us
> > > > > > trying to start link training as well.
> > > > > >
> > > > > > Since any sink with MST is going to support DisplayPort 1.2
> > > > > > anyway,
> > > > > > save
> > > > > > us the hassle of trying to wait until the sink comes back up and
> > > > > > just
> > > > > > never shut the aux block down.
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Fix patch name, no functional changes
> > > > > >
> > > > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > > > Cc: Laura Abbott <[email protected]>
> > > > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > > > Cc: Ville Syrjälä <[email protected]>
> > > > > > Cc: [email protected]
> > > > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to
> > > > > > enable
> > > > > > MST
> > > > > > hub.")
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 62f82c4298ac..0479c377981b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > > > *intel_dp,
> > > > > > int mode)
> > > > > > return;
> > > > > >
> > > > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > > > + unsigned char data = intel_dp->is_mst ?
> > > > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > > > >
> > > > > This smells like a workaround for an actual bug somewhere. Why
> > > > > exactly
> > > > > is the slower wakeup or the AUX block a problem for MST but not for
> > > > > SST
> > > > > when the link training is exactly the same for SST and MST?
> > > >
> > > > I actually thought about this but I still think this is the
> > > > appropriate
> > > > fix.
> > > > So; the real reason for the wakeup not being a problem with SST is
> > > > that
> > > > for
> > > > DPMS on with SST, we actually do a wait to make sure that the hub is
> > > > ready
> > > > before continuing. And yes: I'm fairly sure SST does actually have
> > > > around
> > > > the
> > > > same wakeup time that MST does, but with the wait we do it doesn't
> > > > reallhy
> > > > make a difference. With MST, we could do this but there's a few
> > > > reasons I
> > > > don't think we should:
> > > > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub
> > > > that
> > > > has
> > > > MST is going to be guaranteed to have this.
> > > > * Turning off the aux block means that there's a high chance we're
> > > > going
> > > > to
> > > > miss ESIs from sinks
> > >
> > > And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> > > away if we don't read them within some really short time?
> >
> > That's my hypothesis at least. I'm betting that on the fact that when I
> > was
> > implementing MST retraining before we put the intel_dp_check_mst_status()
> > (or
> > whatever it's called) into the dig workqueue, getting the sink to go down
> > and
> > come back up was a lot more unreliable whenever I introduced anything that
> > would block the esi handler for longer then a very brief period of time
> > (say,
> > 50-100ms?). I've seen some notes elsewhere too that seemed to imply for
> > SST,
> > things were pretty sensitive to irq latency (line 1050, intel_dp.c) so it
> > wouldn't be terribly surprising if it's the same for MST. At the very
> > least,
> > now that we've got the ESI handler running in the dig worker things seem
> > to
> > have gotten a /lot/ more reliable now that we can basically go the whole
> > modeset without blocking the ESI handler for very long.
>
> Hmm. OK, so the spec seems to be saying that we have 100ms to read
> the UP_REQ/DOWN_REPLY msg after the IRQ_HPD. That's still a lot more
> than the 1ms max allowed wakeup time. Looks like there's a extended
> wakeup time request/grant mechanism now, but without the explicit
> grant (which we don't do) the 1ms still holds.
mm, that is true. There were definitely interrupts getting dropped though with
this patch, although it was rather rare. The other thing too (and this was a
lot easier to reproduce) was that when we do a modeset that lowers the link
rate:
* check modeset
* commit disables so we can reprogram the vcpi (puts sink into D3)
* commit enables
* actually send request to put sink into D0
* Go to start link training...
* *TIMEOUT on response from hub*
* continue modeset...
* commit finishes
* short period of time after the commit finishes, we receive the response
from the hub where we tried to start link training
This being said: I think it may actually be a good idea for us to consider
waiting for the hub anyway when we're turning it on like we do with SST, since
we could be waking it up from a D3 state we didn't set ourselves anyway, but
either way I think since we can at least benefit from not having to deal with
the wakeup time at all in modesets like this (and it's not really problematic
to other hubs), we probably should do that as well with or without a busy
wait.
>
> > >
> > > > * It's faster to keep the aux block on anyway
> > > >
> > > >
> > > > >
> > > > > > +
> > > > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > > > return;
> > > > > >
> > > > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > > DP_SET_POWER,
> > > > > > - DP_SET_POWER_D3);
> > > > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > > DP_SET_POWER,
> > > > > > data);
> > > > > > } else {
> > > > > > struct intel_lspcon *lspcon =
> > > > > > dp_to_lspcon(intel_dp);
> > > > > >
> > > > > > --
> > > > > > 2.14.3
> > > > >
> > > > >
> > > >
> > > > --
> > > > Cheers,
> > > > Lyude Paul
> > >
> > >
> >
> > --
> > Cheers,
> > Lyude Paul
>
>
--
Cheers,
Lyude Paul
On Wed, 2018-04-04 at 22:35 +0300, Ville Syrjälä wrote:
> On Wed, Apr 04, 2018 at 03:00:12PM -0400, Lyude Paul wrote:
> > On Wed, 2018-04-04 at 21:53 +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
> > > > On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
> > > > > On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
> > > > > > While enabling/disabling DPMS before link training with MST hubs
> > > > > > is
> > > > > > perfectly valid; unfortunately disabling DPMS results in some
> > > > > > devices
> > > > > > disabling their AUX CH block as well. For SST this isn't as much
> > > > > > of a
> > > > > > problem, but for MST we need to be able to continue handling aux
> > > > > > transactions even when none of the sinks are turned on since it's
> > > > > > possible for us to have a single atomic commit which results in
> > > > > > disabling each downstream sink, followed by subsequently re-
> > > > > > enabling
> > > > > > each sink.
> > > > > >
> > > > > > If we don't do this, we'll end up stalling any pending ESI
> > > > > > interrupts
> > > > > > from the sink for up to 1ms. Unfortunately, dropping ESIs during
> > > > > > this
> > > > > > timespan makes it so that link fallback retraining for MST (which
> > > > > > I
> > > > > > will
> > > > > > be submitting to the ML shortly) fails due to the channel EQ
> > > > > > failure
> > > > > > interrupts potentially getting dropped. Additionally, when
> > > > > > performing
> > > > > > a
> > > > > > modeset that brings the hub status's link status from bad -> good
> > > > > > having
> > > > > > ESIs disabled for that long causes us to miss the hub's response
> > > > > > to us
> > > > > > trying to start link training as well.
> > > > > >
> > > > > > Since any sink with MST is going to support DisplayPort 1.2
> > > > > > anyway,
> > > > > > save
> > > > > > us the hassle of trying to wait until the sink comes back up and
> > > > > > just
> > > > > > never shut the aux block down.
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Fix patch name, no functional changes
> > > > > >
> > > > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > > > Cc: Laura Abbott <[email protected]>
> > > > > > Cc: Dhinakaran Pandiyan <[email protected]>
> > > > > > Cc: Ville Syrjälä <[email protected]>
> > > > > > Cc: [email protected]
> > > > > > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to
> > > > > > enable
> > > > > > MST
> > > > > > hub.")
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++--
> > > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 62f82c4298ac..0479c377981b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp
> > > > > > *intel_dp,
> > > > > > int mode)
> > > > > > return;
> > > > > >
> > > > > > if (mode != DRM_MODE_DPMS_ON) {
> > > > > > + unsigned char data = intel_dp->is_mst ?
> > > > > > + DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;
> > > > >
> > > > > This smells like a workaround for an actual bug somewhere. Why
> > > > > exactly
> > > > > is the slower wakeup or the AUX block a problem for MST but not for
> > > > > SST
> > > > > when the link training is exactly the same for SST and MST?
> > > >
> > > > I actually thought about this but I still think this is the
> > > > appropriate
> > > > fix.
> > > > So; the real reason for the wakeup not being a problem with SST is
> > > > that
> > > > for
> > > > DPMS on with SST, we actually do a wait to make sure that the hub is
> > > > ready
> > > > before continuing. And yes: I'm fairly sure SST does actually have
> > > > around
> > > > the
> > > > same wakeup time that MST does, but with the wait we do it doesn't
> > > > reallhy
> > > > make a difference. With MST, we could do this but there's a few
> > > > reasons I
> > > > don't think we should:
> > > > * We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub
> > > > that
> > > > has
> > > > MST is going to be guaranteed to have this.
> > > > * Turning off the aux block means that there's a high chance we're
> > > > going
> > > > to
> > > > miss ESIs from sinks
> > >
> > > And how exactly do we lose irqs? The hub/whatever throws the up req msgs
> > > away if we don't read them within some really short time?
> >
> > Oh-additionally I did forget to mention that i have actually witnessed the
> > channel eq failures in the ESI getting dropped without this patch.
>
> Not sure what that means. I don't think there is any sideband messaging
> involved in link training so not sure what is dropped in this case. The
> link status/etc. are just polled directly by the upstream device.
no, no nononono they are not always with MST. mdnavare is right regarding the
channel EQ occasionally being the only indicator that there's something wrong
with the link training. I've seen this with my caldigit ts3 with the mst hub
hooked up to it. I'm not entirely sure why, but my guess would be that there's
a displayport repeater somewhere in the TS3 that's not an mst branch device.
I'd think chances are that from the source's perspective, we might be doing
link training with that instead of the actual hub, which means that if link
training between the hub and said repeater failed the only way it would be
reported would be through the channel eq because it's detected by the MST hub.
There's some other behavior regarding this that makes me a little more sure of
this. I've got an old Dell P2415Qb monitor with MST on it that actually does
manage to train at the full 5.4 GBit/s on the caldigit TS3 without needing
fallback retraining. Keep in mind, this monitor is kind of infamous across our
office for having a lot of probably-against-spec MST bugs that don't really
happen on other devices. That being said, it trains and doesn't ever throw
channel EQ failed notifications. But unlike the EVGA MST hub I have which does
throw the EQ failed notifications, the screen flickers so often it's really
difficult for me to believe it's actually link trained properly. This is
mainly where my theory of link status problems that aren't actually visible to
the source comes in, because if that's the case then the most likely
explanation is that the problematic P2415Qb never sends channel EQ failures
because it's firmware just isn't smart enough to monitor the symbol failure
rate and detect that link retraining is required.
>
> > Meaning if
> > we miss them, there's a chance the hub may just not choose to send them
> > again
> > for whatever reason.
> > >
> > > > * It's faster to keep the aux block on anyway
> > > >
> > > >
> > > > >
> > > > > > +
> > > > > > if (downstream_hpd_needs_d0(intel_dp))
> > > > > > return;
> > > > > >
> > > > > > - ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > > DP_SET_POWER,
> > > > > > - DP_SET_POWER_D3);
> > > > > > + ret = drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > > DP_SET_POWER,
> > > > > > data);
> > > > > > } else {
> > > > > > struct intel_lspcon *lspcon =
> > > > > > dp_to_lspcon(intel_dp);
> > > > > >
> > > > > > --
> > > > > > 2.14.3
> > > > >
> > > > >
> > > >
> > > > --
> > > > Cheers,
> > > > Lyude Paul
> > >
> > >
> >
> > --
> > Cheers,
> > Lyude Paul
>
>
--
Cheers,
Lyude Paul