diff mbox

i915/dp_mst: Keep AUX block running when disabling DPMS

Message ID 20180402212142.19841-1-lyude@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lyude Paul April 2, 2018, 9:21 p.m. UTC
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 <lyude@redhat.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
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(-)

Comments

Dhinakaran Pandiyan April 2, 2018, 10:30 p.m. UTC | #1
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 <lyude@redhat.com>

> Cc: Laura Abbott <labbott@redhat.com>

> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

> Cc: stable@vger.kernel.org

> 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);

>
Sasha Levin April 5, 2018, 4:42 p.m. UTC | #2
Hi.

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag.
fixing commit: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.").

The bot has also determined it's probably a bug fixing patch. (score: 98.3082)

The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, v4.4.126, 

v4.15.15: Failed to apply! Possible dependencies:
    Unable to calculate.

v4.14.32: Failed to apply! Possible dependencies:
    Unable to calculate.

v4.9.92: Failed to apply! Possible dependencies:
    Unable to calculate.

v4.4.126: Failed to apply! Possible dependencies:
    Unable to calculate.


--
Thanks.
Sasha
diff mbox

Patch

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);