| Message ID | 20211018094154.1407705-3-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, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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. 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@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > 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);
On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: > On Mon, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > 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
On Tue, 19 Oct 2021, Imre Deak <imre.deak@intel.com> wrote: > On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: >> On Mon, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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. Modeset on a disconnected DP? How? BR, Jani. > >> 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@linux.intel.com> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > Cc: <stable@vger.kernel.org> >> > Signed-off-by: Imre Deak <imre.deak@intel.com> >> > --- >> > 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
On Tue, Oct 19, 2021 at 10:37:33AM +0300, Jani Nikula wrote: > On Tue, 19 Oct 2021, Imre Deak <imre.deak@intel.com> wrote: > > On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: > >> On Mon, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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. > > Modeset on a disconnected DP? How? Yes, just do a modeset on it. It doesn't have to be disconnected either, you can modeset a DP connector before probing it. > > BR, > Jani. > > > > > >> 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@linux.intel.com> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > Cc: <stable@vger.kernel.org> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> > >> > --- > >> > 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 > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, Oct 19, 2021 at 10:39:08AM +0300, Imre Deak wrote: > On Tue, Oct 19, 2021 at 10:37:33AM +0300, Jani Nikula wrote: > > On Tue, 19 Oct 2021, Imre Deak <imre.deak@intel.com> wrote: > > > On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: > > >> On Mon, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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. > > > > Modeset on a disconnected DP? How? > > Yes, just do a modeset on it. It doesn't have to be disconnected either, > you can modeset a DP connector before probing it. Jani, any objections to merge patches 2-6 as well? In a summary the reasons: - Fix userspace triggerable WARNs(). - Fix undefined behavior triggerring BUG() in UBSAN builds (in addition to the case the first patch fixes). - Validate the DP_MAX_LINK_RATE value we read from DPCD. - It unifies some open-coded functionality (patch 3 and 6). > > BR, > > Jani. > > > > > > > > > >> 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@linux.intel.com> > > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> > Cc: <stable@vger.kernel.org> > > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> > > >> > --- > > >> > 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 > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
On Tue, 19 Oct 2021, Imre Deak <imre.deak@intel.com> wrote: > On Tue, Oct 19, 2021 at 10:39:08AM +0300, Imre Deak wrote: >> On Tue, Oct 19, 2021 at 10:37:33AM +0300, Jani Nikula wrote: >> > On Tue, 19 Oct 2021, Imre Deak <imre.deak@intel.com> wrote: >> > > On Tue, Oct 19, 2021 at 10:27:18AM +0300, Jani Nikula wrote: >> > >> On Mon, 18 Oct 2021, Imre Deak <imre.deak@intel.com> 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. >> > >> > Modeset on a disconnected DP? How? >> >> Yes, just do a modeset on it. It doesn't have to be disconnected either, >> you can modeset a DP connector before probing it. > > Jani, > > any objections to merge patches 2-6 as well? In a summary the reasons: > > - Fix userspace triggerable WARNs(). > - Fix undefined behavior triggerring BUG() in UBSAN builds > (in addition to the case the first patch fixes). > - Validate the DP_MAX_LINK_RATE value we read from DPCD. > - It unifies some open-coded functionality (patch 3 and 6). I have some reservations about adding more stuff that we cache, as well as more functions to call to reset the state... but I don't really have concrete proposals either right now, and this makes forward progress. Ack. BR, Jani. > >> > BR, >> > Jani. >> > >> > >> > > >> > >> 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@linux.intel.com> >> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Cc: <stable@vger.kernel.org> >> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> >> > >> > --- >> > >> > 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 >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center
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);
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. 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@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 8 ++++++++ 1 file changed, 8 insertions(+)