[PATCH 1/6] drm/i915/dp: Skip the HW readout of DPCD on disabled encoders

[PATCH 1/6] drm/i915/dp: Skip the HW readout of DPCD on disabled encoders

[Imre Deak]

Reading out the DP encoders' DPCD during booting or resume is only
required for enabled encoders: such encoders may be modesetted during
the initial commit and the link training this involves depends on an
initialized DPCD. For DDI encoders reading out the DPCD is skipped, do
the same on pre-DDI platforms.

Atm, the first DPCD readout without a sink connected - which is a likely
scneario if the encoder is disabled - leaves intel_dp->num_common_rates
at 0, which resulted in

intel_dp_sync_state()->intel_dp_max_common_rate()

in a

intel_dp->common_rates[-1]

access. This by definition results in an undefined behaviour, though to
my best knowledge in all HW/compiler configurations it actually results
in accessing the array item type value preceding the array. In this
case the preceding value happens to be intel_dp->num_common_rates,
which is 0, so this issue - by luck - didn't cause a user visible
problem.

Nevertheless it's still an undefined behaviour and in CONFIG_UBSAN
builds leads to a kernel BUG() (which revealed this problem for us),
hence CC:stable.

A related problem in case the encoder is enabled but the sink is not
connected or the DPCD readout fails is fixed by the next patch.

v2: Amend the commit message describing the root cause of the
    CONFIG_UBSAN BUG().

Fixes: a532cde31de3 ("drm/i915/tc: Fix TypeC port init/resume time sanitization")
References: https://gitlab.freedesktop.org/drm/intel/-/issues/4297
Reported-and-tested-by: Mat Jonczyk <mat.jonczyk@o2.pl>
Cc: Mat Jonczyk <mat.jonczyk@o2.pl>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Jani Nikula <jani.nikula@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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 9d8132dd4cc5a..23de500d56b52 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2007,6 +2007,9 @@ void intel_dp_sync_state(struct intel_encoder *encoder,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+	if (!crtc_state)
+		return;
+
 	/*
 	 * Don't clobber DPCD if it's been already read out during output
 	 * setup (eDP) or detect.
-- 
2.27.0

[PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Imre Deak]

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

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);
-- 
2.27.0

[PATCH 3/6] drm/i915/dp: Ensure max link params are always valid

[Imre Deak]

Atm until the DPCD for a connector is read the max link rate and lane
count params are invalid. If the connector is modeset, in
intel_dp_compute_config(), intel_dp_common_len_rate_limit(max_link_rate)
will return 0, leading to a intel_dp->common_rates[-1] access.

Fix the above by making sure the max link params are always valid.

The above access leads to an undefined behaviour by definition, though
not causing a user visible problem to my best knowledge, see the previous
patch why. Nevertheless it is an undefined behaviour and it triggers a
BUG() in CONFIG_UBSAN builds, hence CC:stable.

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 | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 153ae944a354b..1935eb49f9574 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1864,6 +1864,12 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
 	intel_dp->lane_count = lane_count;
 }
 
+static void intel_dp_reset_max_link_params(struct intel_dp *intel_dp)
+{
+	intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
+	intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
+}
+
 /* Enable backlight PWM and backlight PP control. */
 void intel_edp_backlight_on(const struct intel_crtc_state *crtc_state,
 			    const struct drm_connector_state *conn_state)
@@ -2023,8 +2029,7 @@ void intel_dp_sync_state(struct intel_encoder *encoder,
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		intel_dp_get_dpcd(intel_dp);
 
-	intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
-	intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
+	intel_dp_reset_max_link_params(intel_dp);
 }
 
 bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
@@ -2597,6 +2602,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 		intel_dp_set_sink_rates(intel_dp);
 
 	intel_dp_set_common_rates(intel_dp);
+	intel_dp_reset_max_link_params(intel_dp);
 
 	/* Read the eDP DSC DPCD registers */
 	if (DISPLAY_VER(dev_priv) >= 10)
@@ -4338,12 +4344,7 @@ intel_dp_detect(struct drm_connector *connector,
 	 * supports link training fallback params.
 	 */
 	if (intel_dp->reset_link_params || intel_dp->is_mst) {
-		/* Initial max link lane count */
-		intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
-
-		/* Initial max link rate */
-		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
-
+		intel_dp_reset_max_link_params(intel_dp);
 		intel_dp->reset_link_params = false;
 	}
 
@@ -5011,6 +5012,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_common_rates(intel_dp);
+	intel_dp_reset_max_link_params(intel_dp);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		intel_dp->pps.active_pipe = vlv_active_pipe(intel_dp);
-- 
2.27.0

[PATCH v2 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Imre Deak]

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.

v2: Clear the default sink rates, before initialzing these for eDP.

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 23de500d56b52..9cbe85746fc41 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)
 {
@@ -2556,6 +2562,9 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 	 */
 	intel_psr_init_dpcd(intel_dp);
 
+	/* Clear the default sink rates */
+	intel_dp->num_sink_rates = 0;
+
 	/* Read the eDP 1.4+ supported link rates. */
 	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
 		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
@@ -5003,6 +5012,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);
-- 
2.27.0

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Imre Deak]

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

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Imre Deak]

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

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Imre Deak]

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

Re: [Intel-gfx] [PATCH 2/6] drm/i915/dp: Ensure sink rate values are always valid

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center