diff mbox series

[18/19] drm/i915/dp: Suspend/resume DP tunnels

Message ID 20240123102850.390126-19-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Add Display Port tunnel BW allocation support | expand

Commit Message

Imre Deak Jan. 23, 2024, 10:28 a.m. UTC
Suspend and resume DP tunnels during system suspend/resume, disabling
the BW allocation mode during suspend, re-enabling it after resume. This
reflects the link's BW management component (Thunderbolt CM) disabling
BWA during suspend. Before any BW requests the driver must read the
sink's DPRX capabilities (since the BW manager requires this
information, so snoops for it on AUX), so ensure this read takes place.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Ville Syrjälä Jan. 31, 2024, 4:18 p.m. UTC | #1
On Tue, Jan 23, 2024 at 12:28:49PM +0200, Imre Deak wrote:
> Suspend and resume DP tunnels during system suspend/resume, disabling
> the BW allocation mode during suspend, re-enabling it after resume. This
> reflects the link's BW management component (Thunderbolt CM) disabling
> BWA during suspend. Before any BW requests the driver must read the
> sink's DPRX capabilities (since the BW manager requires this
> information, so snoops for it on AUX), so ensure this read takes place.

Isn't that going to screw up the age old problem of .compute_config()
potentially failing during the resume modeset if we no longer have
the same amount of bandwidth available as we had before suspend?
So far we've been getting away with this exactly by not updating 
the dpcd stuff before the modeset during resume.

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8ebfb039000f6..bc138a54f8d7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -36,6 +36,7 @@
>  #include <asm/byteorder.h>
>  
>  #include <drm/display/drm_dp_helper.h>
> +#include <drm/display/drm_dp_tunnel.h>
>  #include <drm/display/drm_dsc_helper.h>
>  #include <drm/display/drm_hdmi_helper.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -3320,18 +3321,21 @@ void intel_dp_sync_state(struct intel_encoder *encoder,
>  			 const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> -
> -	if (!crtc_state)
> -		return;
> +	bool dpcd_updated = false;
>  
>  	/*
>  	 * Don't clobber DPCD if it's been already read out during output
>  	 * setup (eDP) or detect.
>  	 */
> -	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
> +	if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) {
>  		intel_dp_get_dpcd(intel_dp);
> +		dpcd_updated = true;
> +	}
>  
> -	intel_dp_reset_max_link_params(intel_dp);
> +	intel_dp_tunnel_resume(intel_dp, dpcd_updated);
> +
> +	if (crtc_state)
> +		intel_dp_reset_max_link_params(intel_dp);
>  }
>  
>  bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
> @@ -5973,6 +5977,8 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
>  	struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder);
>  
>  	intel_pps_vdd_off_sync(intel_dp);
> +
> +	intel_dp_tunnel_suspend(intel_dp);
>  }
>  
>  void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder)
> -- 
> 2.39.2
Imre Deak Jan. 31, 2024, 4:59 p.m. UTC | #2
On Wed, Jan 31, 2024 at 06:18:22PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:49PM +0200, Imre Deak wrote:
> > Suspend and resume DP tunnels during system suspend/resume, disabling
> > the BW allocation mode during suspend, re-enabling it after resume. This
> > reflects the link's BW management component (Thunderbolt CM) disabling
> > BWA during suspend. Before any BW requests the driver must read the
> > sink's DPRX capabilities (since the BW manager requires this
> > information, so snoops for it on AUX), so ensure this read takes place.
> 
> Isn't that going to screw up the age old problem of .compute_config()
> potentially failing during the resume modeset if we no longer have
> the same amount of bandwidth available as we had before suspend?
> So far we've been getting away with this exactly by not updating 
> the dpcd stuff before the modeset during resume.

Right, in the case where this would be a problem (so not counting where
the caps haven't been read out yet and so we update here
intel_dp->dpcd), the caps in intel_dp->dpcd will be preserved, not
actually updated with the read-out values, see intel_dp_tunnel_resume()
in patch 11.

The same goes for the tunnel (group) BW: it will not be updated during
resume (by way of the connector/tunnel detection being blocked during
the restore modeset), so the restore modeset should see the same amount
of BW as there was during suspend.

> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 8ebfb039000f6..bc138a54f8d7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -36,6 +36,7 @@
> >  #include <asm/byteorder.h>
> >  
> >  #include <drm/display/drm_dp_helper.h>
> > +#include <drm/display/drm_dp_tunnel.h>
> >  #include <drm/display/drm_dsc_helper.h>
> >  #include <drm/display/drm_hdmi_helper.h>
> >  #include <drm/drm_atomic_helper.h>
> > @@ -3320,18 +3321,21 @@ void intel_dp_sync_state(struct intel_encoder *encoder,
> >  			 const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > -
> > -	if (!crtc_state)
> > -		return;
> > +	bool dpcd_updated = false;
> >  
> >  	/*
> >  	 * Don't clobber DPCD if it's been already read out during output
> >  	 * setup (eDP) or detect.
> >  	 */
> > -	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
> > +	if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) {
> >  		intel_dp_get_dpcd(intel_dp);
> > +		dpcd_updated = true;
> > +	}
> >  
> > -	intel_dp_reset_max_link_params(intel_dp);
> > +	intel_dp_tunnel_resume(intel_dp, dpcd_updated);
> > +
> > +	if (crtc_state)
> > +		intel_dp_reset_max_link_params(intel_dp);
> >  }
> >  
> >  bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
> > @@ -5973,6 +5977,8 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder);
> >  
> >  	intel_pps_vdd_off_sync(intel_dp);
> > +
> > +	intel_dp_tunnel_suspend(intel_dp);
> >  }
> >  
> >  void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder)
> > -- 
> > 2.39.2
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 8ebfb039000f6..bc138a54f8d7b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -36,6 +36,7 @@ 
 #include <asm/byteorder.h>
 
 #include <drm/display/drm_dp_helper.h>
+#include <drm/display/drm_dp_tunnel.h>
 #include <drm/display/drm_dsc_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/drm_atomic_helper.h>
@@ -3320,18 +3321,21 @@  void intel_dp_sync_state(struct intel_encoder *encoder,
 			 const struct intel_crtc_state *crtc_state)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-
-	if (!crtc_state)
-		return;
+	bool dpcd_updated = false;
 
 	/*
 	 * Don't clobber DPCD if it's been already read out during output
 	 * setup (eDP) or detect.
 	 */
-	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
+	if (crtc_state && intel_dp->dpcd[DP_DPCD_REV] == 0) {
 		intel_dp_get_dpcd(intel_dp);
+		dpcd_updated = true;
+	}
 
-	intel_dp_reset_max_link_params(intel_dp);
+	intel_dp_tunnel_resume(intel_dp, dpcd_updated);
+
+	if (crtc_state)
+		intel_dp_reset_max_link_params(intel_dp);
 }
 
 bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
@@ -5973,6 +5977,8 @@  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
 	struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder);
 
 	intel_pps_vdd_off_sync(intel_dp);
+
+	intel_dp_tunnel_suspend(intel_dp);
 }
 
 void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder)