| Message ID | 20211018094154.1407705-5-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Fix link parameter use in lack of a valid DPCD | expand |
On Mon, Oct 18, 2021 at 12:41:52PM +0300, Imre Deak wrote: > Print an error if the DPCD sink max lane count is invalid and fix it up. > > While at it also add an assert that the link max lane count (derived > from intel_dp_max_common_lane_count(), potentially reduced by the LT > fallback logic) value is also valid. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > .../drm/i915/display/intel_display_types.h | 2 + > drivers/gpu/drm/i915/display/intel_dp.c | 44 ++++++++++++++++++- > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 39e11eaec1a3f..1e42bf901263c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1563,6 +1563,8 @@ struct intel_dp { > int num_sink_rates; > int sink_rates[DP_MAX_SUPPORTED_RATES]; > bool use_rate_select; > + /* Max sink lane count as reported by DP_MAX_LANE_COUNT */ > + int max_sink_lane_count; > /* intersection of source and sink rates */ > int num_common_rates; > int common_rates[DP_MAX_SUPPORTED_RATES]; > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 1935eb49f9574..f7711779df132 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -197,6 +197,35 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > intel_dp->num_sink_rates = i; > } > > +static void intel_dp_set_default_max_sink_lane_count(struct intel_dp *intel_dp) > +{ > + intel_dp->max_sink_lane_count = 1; > +} > + > +static void intel_dp_set_max_sink_lane_count(struct intel_dp *intel_dp) > +{ > + struct intel_connector *connector = intel_dp->attached_connector; > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct intel_encoder *encoder = &intel_dig_port->base; > + > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > + > + switch (intel_dp->max_sink_lane_count) { > + case 1: > + case 2: > + case 4: > + return; > + } > + > + drm_err(&dp_to_i915(intel_dp)->drm, > + "[CONNECTOR:%d:%s][ENCODER:%d:%s] Invalid DPCD max lane count (%d), using default\n", > + connector->base.base.id, connector->base.name, > + encoder->base.base.id, encoder->base.name, > + intel_dp->max_sink_lane_count); > + > + intel_dp_set_default_max_sink_lane_count(intel_dp); > +} > + > /* Get length of rates array potentially limited by max_rate. */ > static int intel_dp_rate_limit_len(const int *rates, int len, int max_rate) > { > @@ -230,7 +259,7 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > { > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > int source_max = dig_port->max_lanes; > - int sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > + int sink_max = intel_dp->max_sink_lane_count; > int fia_max = intel_tc_port_fia_max_lane_count(dig_port); > int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps); > > @@ -242,7 +271,15 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > int intel_dp_max_lane_count(struct intel_dp *intel_dp) > { > - return intel_dp->max_link_lane_count; > + switch (intel_dp->max_link_lane_count) { > + case 1: > + case 2: > + case 4: > + return intel_dp->max_link_lane_count; > + default: > + MISSING_CASE(intel_dp->max_link_lane_count); > + return 1; > + } > } I guess this is just a second level sanity check. I was wondering it gets confusing and people start thinking this can actually happen, but I suppose the MISSING_CASE() should be indication enough that it in fact should not happen. Series looks sane to me: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> BTW there's now some kms_bw thing in igt which seems to forcing DP connectors on left and right, and thus triggering a bunch of WARNs. > > /* > @@ -2600,6 +2637,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > intel_dp->use_rate_select = true; > else > intel_dp_set_sink_rates(intel_dp); > + intel_dp_set_max_sink_lane_count(intel_dp); > > intel_dp_set_common_rates(intel_dp); > intel_dp_reset_max_link_params(intel_dp); > @@ -2645,6 +2683,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > drm_dp_is_branch(intel_dp->dpcd)); > > intel_dp_set_sink_rates(intel_dp); > + intel_dp_set_max_sink_lane_count(intel_dp); > intel_dp_set_common_rates(intel_dp); > } > > @@ -5011,6 +5050,7 @@ intel_dp_init_connector(struct intel_digital_port *dig_port, > > intel_dp_set_source_rates(intel_dp); > intel_dp_set_default_sink_rates(intel_dp); > + intel_dp_set_default_max_sink_lane_count(intel_dp); > intel_dp_set_common_rates(intel_dp); > intel_dp_reset_max_link_params(intel_dp); > > -- > 2.27.0
On Mon, Oct 18, 2021 at 06:04:18PM +0300, Ville Syrjälä wrote: > On Mon, Oct 18, 2021 at 12:41:52PM +0300, Imre Deak wrote: > > Print an error if the DPCD sink max lane count is invalid and fix it up. > > > > While at it also add an assert that the link max lane count (derived > > from intel_dp_max_common_lane_count(), potentially reduced by the LT > > fallback logic) value is also valid. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > .../drm/i915/display/intel_display_types.h | 2 + > > drivers/gpu/drm/i915/display/intel_dp.c | 44 ++++++++++++++++++- > > 2 files changed, 44 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 39e11eaec1a3f..1e42bf901263c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1563,6 +1563,8 @@ struct intel_dp { > > int num_sink_rates; > > int sink_rates[DP_MAX_SUPPORTED_RATES]; > > bool use_rate_select; > > + /* Max sink lane count as reported by DP_MAX_LANE_COUNT */ > > + int max_sink_lane_count; > > /* intersection of source and sink rates */ > > int num_common_rates; > > int common_rates[DP_MAX_SUPPORTED_RATES]; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 1935eb49f9574..f7711779df132 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -197,6 +197,35 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > > intel_dp->num_sink_rates = i; > > } > > > > +static void intel_dp_set_default_max_sink_lane_count(struct intel_dp *intel_dp) > > +{ > > + intel_dp->max_sink_lane_count = 1; > > +} > > + > > +static void intel_dp_set_max_sink_lane_count(struct intel_dp *intel_dp) > > +{ > > + struct intel_connector *connector = intel_dp->attached_connector; > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > + struct intel_encoder *encoder = &intel_dig_port->base; > > + > > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > + > > + switch (intel_dp->max_sink_lane_count) { > > + case 1: > > + case 2: > > + case 4: > > + return; > > + } > > + > > + drm_err(&dp_to_i915(intel_dp)->drm, > > + "[CONNECTOR:%d:%s][ENCODER:%d:%s] Invalid DPCD max lane count (%d), using default\n", > > + connector->base.base.id, connector->base.name, > > + encoder->base.base.id, encoder->base.name, > > + intel_dp->max_sink_lane_count); > > + > > + intel_dp_set_default_max_sink_lane_count(intel_dp); > > +} > > + > > /* Get length of rates array potentially limited by max_rate. */ > > static int intel_dp_rate_limit_len(const int *rates, int len, int max_rate) > > { > > @@ -230,7 +259,7 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > { > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > int source_max = dig_port->max_lanes; > > - int sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > + int sink_max = intel_dp->max_sink_lane_count; > > int fia_max = intel_tc_port_fia_max_lane_count(dig_port); > > int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps); > > > > @@ -242,7 +271,15 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > > > int intel_dp_max_lane_count(struct intel_dp *intel_dp) > > { > > - return intel_dp->max_link_lane_count; > > + switch (intel_dp->max_link_lane_count) { > > + case 1: > > + case 2: > > + case 4: > > + return intel_dp->max_link_lane_count; > > + default: > > + MISSING_CASE(intel_dp->max_link_lane_count); > > + return 1; > > + } > > } > > I guess this is just a second level sanity check. I was wondering it > gets confusing and people start thinking this can actually happen, > but I suppose the MISSING_CASE() should be indication enough that it > in fact should not happen. Yes it shouldn't happen. Given that we don't consider the FIA reg value external, but I think that's a reasonable assumption. > Series looks sane to me: > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks. > BTW there's now some kms_bw thing in igt which seems to forcing > DP connectors on left and right, and thus triggering a bunch of > WARNs. Yes, noticed. This series should fix the WARN, however the modeset will still fail, due to using the minimum link_rate/lane_count set by default in this patchset. But due to the LT failing and fallback reducing the link params it would anyway fail after the first modeset. I wonder what would be a good solution if the above kind of use case is important enough (I at least use this for debugging). Maybe a virtual loopback connector which could handle LT (in kernel perhaps) and even any AUX traffic (in userspace)? Anyway that's far beyond the scope of this patchset. > > /* > > @@ -2600,6 +2637,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > > intel_dp->use_rate_select = true; > > else > > intel_dp_set_sink_rates(intel_dp); > > + intel_dp_set_max_sink_lane_count(intel_dp); > > > > intel_dp_set_common_rates(intel_dp); > > intel_dp_reset_max_link_params(intel_dp); > > @@ -2645,6 +2683,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > drm_dp_is_branch(intel_dp->dpcd)); > > > > intel_dp_set_sink_rates(intel_dp); > > + intel_dp_set_max_sink_lane_count(intel_dp); > > intel_dp_set_common_rates(intel_dp); > > } > > > > @@ -5011,6 +5050,7 @@ intel_dp_init_connector(struct intel_digital_port *dig_port, > > > > intel_dp_set_source_rates(intel_dp); > > intel_dp_set_default_sink_rates(intel_dp); > > + intel_dp_set_default_max_sink_lane_count(intel_dp); > > intel_dp_set_common_rates(intel_dp); > > intel_dp_reset_max_link_params(intel_dp); > > > > -- > > 2.27.0 > > -- > Ville Syrjälä > Intel
On Mon, Oct 18, 2021 at 06:13:19PM +0300, Imre Deak wrote: > On Mon, Oct 18, 2021 at 06:04:18PM +0300, Ville Syrjälä wrote: > > On Mon, Oct 18, 2021 at 12:41:52PM +0300, Imre Deak wrote: > > > Print an error if the DPCD sink max lane count is invalid and fix it up. > > > > > > While at it also add an assert that the link max lane count (derived > > > from intel_dp_max_common_lane_count(), potentially reduced by the LT > > > fallback logic) value is also valid. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > .../drm/i915/display/intel_display_types.h | 2 + > > > drivers/gpu/drm/i915/display/intel_dp.c | 44 ++++++++++++++++++- > > > 2 files changed, 44 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 39e11eaec1a3f..1e42bf901263c 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1563,6 +1563,8 @@ struct intel_dp { > > > int num_sink_rates; > > > int sink_rates[DP_MAX_SUPPORTED_RATES]; > > > bool use_rate_select; > > > + /* Max sink lane count as reported by DP_MAX_LANE_COUNT */ > > > + int max_sink_lane_count; > > > /* intersection of source and sink rates */ > > > int num_common_rates; > > > int common_rates[DP_MAX_SUPPORTED_RATES]; > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 1935eb49f9574..f7711779df132 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -197,6 +197,35 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > > > intel_dp->num_sink_rates = i; > > > } > > > > > > +static void intel_dp_set_default_max_sink_lane_count(struct intel_dp *intel_dp) > > > +{ > > > + intel_dp->max_sink_lane_count = 1; > > > +} > > > + > > > +static void intel_dp_set_max_sink_lane_count(struct intel_dp *intel_dp) > > > +{ > > > + struct intel_connector *connector = intel_dp->attached_connector; > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > + struct intel_encoder *encoder = &intel_dig_port->base; > > > + > > > + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > > > + > > > + switch (intel_dp->max_sink_lane_count) { > > > + case 1: > > > + case 2: > > > + case 4: > > > + return; > > > + } > > > + > > > + drm_err(&dp_to_i915(intel_dp)->drm, > > > + "[CONNECTOR:%d:%s][ENCODER:%d:%s] Invalid DPCD max lane count (%d), using default\n", > > > + connector->base.base.id, connector->base.name, > > > + encoder->base.base.id, encoder->base.name, > > > + intel_dp->max_sink_lane_count); > > > + > > > + intel_dp_set_default_max_sink_lane_count(intel_dp); > > > +} > > > + > > > /* Get length of rates array potentially limited by max_rate. */ > > > static int intel_dp_rate_limit_len(const int *rates, int len, int max_rate) > > > { > > > @@ -230,7 +259,7 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > > { > > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > > int source_max = dig_port->max_lanes; > > > - int sink_max = drm_dp_max_lane_count(intel_dp->dpcd); > > > + int sink_max = intel_dp->max_sink_lane_count; > > > int fia_max = intel_tc_port_fia_max_lane_count(dig_port); > > > int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps); > > > > > > @@ -242,7 +271,15 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > > > > > int intel_dp_max_lane_count(struct intel_dp *intel_dp) > > > { > > > - return intel_dp->max_link_lane_count; > > > + switch (intel_dp->max_link_lane_count) { > > > + case 1: > > > + case 2: > > > + case 4: > > > + return intel_dp->max_link_lane_count; > > > + default: > > > + MISSING_CASE(intel_dp->max_link_lane_count); > > > + return 1; > > > + } > > > } > > > > I guess this is just a second level sanity check. I was wondering it > > gets confusing and people start thinking this can actually happen, > > but I suppose the MISSING_CASE() should be indication enough that it > > in fact should not happen. > > Yes it shouldn't happen. Given that we don't consider the FIA reg value > external, but I think that's a reasonable assumption. > > > Series looks sane to me: > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks. > > > BTW there's now some kms_bw thing in igt which seems to forcing > > DP connectors on left and right, and thus triggering a bunch of > > WARNs. > > Yes, noticed. This series should fix the WARN, however the modeset will > still fail, due to using the minimum link_rate/lane_count set by default > in this patchset. But due to the LT failing and fallback reducing the > link params it would anyway fail after the first modeset. I wonder what > would be a good solution if the above kind of use case is important > enough (I at least use this for debugging). Maybe a virtual loopback > connector which could handle LT (in kernel perhaps) and even any AUX > traffic (in userspace)? Anyway that's far beyond the scope of this > patchset. Yeah, a virtual DP sink with DPCD and the works is something I've occasionally pondered. But it would take some actual work.
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 39e11eaec1a3f..1e42bf901263c 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1563,6 +1563,8 @@ struct intel_dp { int num_sink_rates; int sink_rates[DP_MAX_SUPPORTED_RATES]; bool use_rate_select; + /* Max sink lane count as reported by DP_MAX_LANE_COUNT */ + int max_sink_lane_count; /* intersection of source and sink rates */ int num_common_rates; int common_rates[DP_MAX_SUPPORTED_RATES]; diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 1935eb49f9574..f7711779df132 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -197,6 +197,35 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) intel_dp->num_sink_rates = i; } +static void intel_dp_set_default_max_sink_lane_count(struct intel_dp *intel_dp) +{ + intel_dp->max_sink_lane_count = 1; +} + +static void intel_dp_set_max_sink_lane_count(struct intel_dp *intel_dp) +{ + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct intel_encoder *encoder = &intel_dig_port->base; + + intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); + + switch (intel_dp->max_sink_lane_count) { + case 1: + case 2: + case 4: + return; + } + + drm_err(&dp_to_i915(intel_dp)->drm, + "[CONNECTOR:%d:%s][ENCODER:%d:%s] Invalid DPCD max lane count (%d), using default\n", + connector->base.base.id, connector->base.name, + encoder->base.base.id, encoder->base.name, + intel_dp->max_sink_lane_count); + + intel_dp_set_default_max_sink_lane_count(intel_dp); +} + /* Get length of rates array potentially limited by max_rate. */ static int intel_dp_rate_limit_len(const int *rates, int len, int max_rate) { @@ -230,7 +259,7 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) { struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); int source_max = dig_port->max_lanes; - int sink_max = drm_dp_max_lane_count(intel_dp->dpcd); + int sink_max = intel_dp->max_sink_lane_count; int fia_max = intel_tc_port_fia_max_lane_count(dig_port); int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps); @@ -242,7 +271,15 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) int intel_dp_max_lane_count(struct intel_dp *intel_dp) { - return intel_dp->max_link_lane_count; + switch (intel_dp->max_link_lane_count) { + case 1: + case 2: + case 4: + return intel_dp->max_link_lane_count; + default: + MISSING_CASE(intel_dp->max_link_lane_count); + return 1; + } } /* @@ -2600,6 +2637,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) intel_dp->use_rate_select = true; else intel_dp_set_sink_rates(intel_dp); + intel_dp_set_max_sink_lane_count(intel_dp); intel_dp_set_common_rates(intel_dp); intel_dp_reset_max_link_params(intel_dp); @@ -2645,6 +2683,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) drm_dp_is_branch(intel_dp->dpcd)); intel_dp_set_sink_rates(intel_dp); + intel_dp_set_max_sink_lane_count(intel_dp); intel_dp_set_common_rates(intel_dp); } @@ -5011,6 +5050,7 @@ intel_dp_init_connector(struct intel_digital_port *dig_port, intel_dp_set_source_rates(intel_dp); intel_dp_set_default_sink_rates(intel_dp); + intel_dp_set_default_max_sink_lane_count(intel_dp); intel_dp_set_common_rates(intel_dp); intel_dp_reset_max_link_params(intel_dp);
Print an error if the DPCD sink max lane count is invalid and fix it up. While at it also add an assert that the link max lane count (derived from intel_dp_max_common_lane_count(), potentially reduced by the LT fallback logic) value is also valid. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- .../drm/i915/display/intel_display_types.h | 2 + drivers/gpu/drm/i915/display/intel_dp.c | 44 ++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-)