On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: > On Mon, 18 Oct 2021, Imre Deak <imre.deak@xxxxxxxxx> wrote: > > Atm, there are no sink rate values set for DP (vs. eDP) sinks until the > > DPCD capabilities are successfully read from the sink. During this time > > intel_dp->num_common_rates is 0 which can lead to a > > > > intel_dp->common_rates[-1] (*) > > > > access, which is an undefined behaviour, in the following cases: > > > > - In intel_dp_sync_state(), if the encoder is enabled without a sink > > connected to the encoder's connector (BIOS enabled a monitor, but the > > user unplugged the monitor until the driver loaded). > > - In intel_dp_sync_state() if the encoder is enabled with a sink > > connected, but for some reason the DPCD read has failed. > > - In intel_dp_compute_link_config() if modesetting a connector without > > a sink connected on it. > > - In intel_dp_compute_link_config() if modesetting a connector with a > > a sink connected on it, but before probing the connector first. > > > > To avoid the (*) access in all the above cases, make sure that the sink > > rate table - and hence the common rate table - is always valid, by > > setting a default minimum sink rate when registering the connector > > before anything could use it. > > > > I also considered setting all the DP link rates by default, so that > > modesetting with higher resolution modes also succeeds in the last two > > cases above. However in case a sink is not connected that would stop > > working after the first modeset, due to the LT fallback logic. So this > > would need more work, beyond the scope of this fix. > > > > As I mentioned in the previous patch, I don't think the issue this patch > > fixes is user visible, however it is an undefined behaviour by > > definition and triggers a BUG() in CONFIG_UBSAN builds, hence CC:stable. > > I think the question here, and in the following patches, is whether this > papers over potential bugs elsewhere. > > Would the original bug fixed by patch 1 have been detected if all the > safeguards here had been in place? Point being, we shouldn't be doing > any of these things before we've read the dpcd. Modesets are possible even without a connected sink or a read-out DPCD, so the link parameters need to be valid even without those. > BR, > Jani. > > > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4297 > > References: https://gitlab.freedesktop.org/drm/intel/-/issues/4298 > > Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: <stable@xxxxxxxxxxxxxxx> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 23de500d56b52..153ae944a354b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -120,6 +120,12 @@ bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state) > > return crtc_state->port_clock >= 1000000; > > } > > > > +static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp) > > +{ > > + intel_dp->sink_rates[0] = 162000; > > + intel_dp->num_sink_rates = 1; > > +} > > + > > /* update sink rates from dpcd */ > > static void intel_dp_set_sink_rates(struct intel_dp *intel_dp) > > { > > @@ -5003,6 +5009,8 @@ 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_common_rates(intel_dp); > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp); > > -- > Jani Nikula, Intel Open Source Graphics Center
