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