[PATCH 0/2] drm/probe_helper, i915: Validate MST modes against PBN limits

[PATCH 0/2] drm/probe_helper, i915: Validate MST modes against PBN limits

[Lyude Paul]

Something we've been missing for a while with drivers that support MST
is being able to prune modes that can't be set due to bandwidth
limitations. So, let's go ahead and add that. This also adds a new hook
that was needed, mode_valid_ctx, so that we can grab additional locks as
needed when validating modes.

Lee Shawn C (1):
  drm/i915/mst: filter out the display mode exceed sink's capability

Lyude Paul (1):
  drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

 drivers/gpu/drm/drm_crtc_helper_internal.h  |  6 +-
 drivers/gpu/drm/drm_probe_helper.c          | 65 ++++++++++++++-------
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 39 +++++++++++--
 include/drm/drm_modeset_helper_vtables.h    | 41 +++++++++++++
 4 files changed, 123 insertions(+), 28 deletions(-)

-- 
2.26.2

[PATCH 1/2] drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

[Lyude Paul]

This is just an atomic version of mode_valid, which is intended to be
used for situations where a driver might need to check the atomic state
of objects other than the connector itself. One such example is with
MST, where the maximum possible bandwidth on a connector can change
dynamically irregardless of the display configuration.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Lee Shawn C <shawn.c.lee@intel.com>
Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/drm_crtc_helper_internal.h |  6 +-
 drivers/gpu/drm/drm_probe_helper.c         | 65 ++++++++++++++--------
 include/drm/drm_modeset_helper_vtables.h   | 41 ++++++++++++++
 3 files changed, 88 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
index f0a66ef47e5ad..ca767cba6094d 100644
--- a/drivers/gpu/drm/drm_crtc_helper_internal.h
+++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
@@ -73,8 +73,10 @@ enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
 					 const struct drm_display_mode *mode);
 enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
 					    const struct drm_display_mode *mode);
-enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
-					      struct drm_display_mode *mode);
+enum drm_mode_status
+drm_connector_mode_valid(struct drm_connector *connector,
+			 struct drm_display_mode *mode,
+			 struct drm_modeset_acquire_ctx *ctx);
 
 struct drm_encoder *
 drm_connector_get_single_encoder(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 466dfbba82564..3132784736841 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -86,16 +86,17 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
 	return MODE_OK;
 }
 
-static enum drm_mode_status
+static int
 drm_mode_validate_pipeline(struct drm_display_mode *mode,
-			    struct drm_connector *connector)
+			   struct drm_connector *connector,
+			   struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_device *dev = connector->dev;
-	enum drm_mode_status ret = MODE_OK;
 	struct drm_encoder *encoder;
+	int ret = MODE_OK;
 
 	/* Step 1: Validate against connector */
-	ret = drm_connector_mode_valid(connector, mode);
+	ret = drm_connector_mode_valid(connector, mode, ctx);
 	if (ret != MODE_OK)
 		return ret;
 
@@ -196,16 +197,23 @@ enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
 	return encoder_funcs->mode_valid(encoder, mode);
 }
 
-enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
-					      struct drm_display_mode *mode)
+int
+drm_connector_mode_valid(struct drm_connector *connector,
+			 struct drm_display_mode *mode,
+			 struct drm_modeset_acquire_ctx *ctx)
 {
 	const struct drm_connector_helper_funcs *connector_funcs =
 		connector->helper_private;
 
-	if (!connector_funcs || !connector_funcs->mode_valid)
+	if (!connector_funcs)
 		return MODE_OK;
 
-	return connector_funcs->mode_valid(connector, mode);
+	if (connector_funcs->mode_valid_ctx)
+		return connector_funcs->mode_valid_ctx(connector, mode, ctx);
+	else if (connector_funcs->mode_valid)
+		return connector_funcs->mode_valid(connector, mode);
+	else
+		return MODE_OK;
 }
 
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
@@ -375,8 +383,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
  *      (if specified)
  *    - drm_mode_validate_flag() checks the modes against basic connector
  *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
- *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
- *      driver and/or sink specific checks
+ *    - the optional &drm_connector_helper_funcs.mode_valid or
+ *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform driver
+ *      and/or sink specific checks
  *    - the optional &drm_crtc_helper_funcs.mode_valid,
  *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
  *      helpers can perform driver and/or source specific checks which are also
@@ -507,22 +516,34 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		mode_flags |= DRM_MODE_FLAG_3D_MASK;
 
 	list_for_each_entry(mode, &connector->modes, head) {
-		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_driver(dev, mode);
+		if (mode->status != MODE_OK)
+			continue;
+
+		mode->status = drm_mode_validate_driver(dev, mode);
+		if (mode->status != MODE_OK)
+			continue;
 
-		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_size(mode, maxX, maxY);
+		mode->status = drm_mode_validate_size(mode, maxX, maxY);
+		if (mode->status != MODE_OK)
+			continue;
 
-		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_flag(mode, mode_flags);
+		mode->status = drm_mode_validate_flag(mode, mode_flags);
+		if (mode->status != MODE_OK)
+			continue;
 
-		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_pipeline(mode,
-								  connector);
+		ret = drm_mode_validate_pipeline(mode, connector, &ctx);
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			goto retry;
+		} else if (WARN_ON_ONCE(ret < 0)) {
+			mode->status = MODE_BAD;
+		} else {
+			mode->status = ret;
+		}
 
-		if (mode->status == MODE_OK)
-			mode->status = drm_mode_validate_ycbcr420(mode,
-								  connector);
+		if (mode->status != MODE_OK)
+			continue;
+		mode->status = drm_mode_validate_ycbcr420(mode, connector);
 	}
 
 prune:
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 421a30f084631..8f020c3424b2b 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -968,6 +968,47 @@ struct drm_connector_helper_funcs {
 	 */
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
 					   struct drm_display_mode *mode);
+
+	/**
+	 * @mode_valid_ctx:
+	 *
+	 * Callback to validate a mode for a connector, irrespective of the
+	 * specific display configuration.
+	 *
+	 * This callback is used by the probe helpers to filter the mode list
+	 * (which is usually derived from the EDID data block from the sink).
+	 * See e.g. drm_helper_probe_single_connector_modes().
+	 *
+	 * This function is optional, and is the atomic version of
+	 * &drm_connector_funcs.mode_valid.
+	 *
+	 * To allow for accessing the atomic state of modesetting objects, the
+	 * helper libraries always call this with ctx set to a valid context,
+	 * and &drm_mode_config.connection_mutex will always be locked with
+	 * the ctx parameter set to @ctx. This allows for taking additional
+	 * locks as required.
+	 *
+	 * Even though additional locks may be acquired, this callback is
+	 * still expected not to take any constraints into account which would
+	 * be influenced by the currently set display state - such constraints
+	 * should be handled in the driver's atomic check. For example, if a
+	 * connector shares display bandwidth with other connectors then it
+	 * would be ok to validate a mode uses against the maximum possible
+	 * bandwidth of the connector. But it wouldn't be ok to take the
+	 * current bandwidth usage of other connectors into account, as this
+	 * would change depending on the display state.
+	 *
+	 * Returns:
+	 *
+	 * Either &drm_mode_status.MODE_OK, one of the failure reasons in
+	 * &enum drm_mode_status, or -EDEADLK if a deadlock would have
+	 * occurred and we need to backoff.
+	 *
+	 */
+	int (*mode_valid_ctx)(struct drm_connector *connector,
+			      struct drm_display_mode *mode,
+			      struct drm_modeset_acquire_ctx *ctx);
+
 	/**
 	 * @best_encoder:
 	 *
-- 
2.26.2

[PATCH 2/2] drm/i915/mst: filter out the display mode exceed sink's capability

[Lyude Paul]

From: Lee Shawn C <shawn.c.lee@intel.com>

So far, max dot clock rate for MST mode rely on physcial
bandwidth limitation. It would caused compatibility issue
if source display resolution exceed MST hub output ability.

For example, source DUT had DP 1.2 output capability.
And MST docking just support HDMI 1.4 spec. When a HDMI 2.0
monitor connected. Source would retrieve EDID from external
and get max resolution 4k@60fps. DP 1.2 can support 4K@60fps
because it did not surpass DP physical bandwidth limitation.
Do modeset to 4k@60fps, source output display data but MST
docking can't output HDMI properly due to this resolution
already over HDMI 1.4 spec.

Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn
instead of available_pbn for bandwidth checks").
Source driver should refer to full_pbn to evaluate sink
output capability. And filter out the resolution surpass
sink output limitation.

v2: Using mgr->base.lock to protect full_pbn.
v3: Add ctx lock.
v4:
* s/intel_dp_mst_mode_clock_exceed_pbn_bandwidth/
  intel_dp_mst_mode_clock_exceeds_pbn_bw/
* Use the new drm_connector_helper_funcs.mode_valid_ctx to properly pipe
  down the drm_modeset_acquire_ctx that the probe helpers are using, so
  we can safely grab &mgr->base.lock without deadlocking

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Co-developed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 39 ++++++++++++++++++---
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index d18b406f2a7d2..cf052095ad785 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -610,15 +610,42 @@ static int intel_dp_mst_get_modes(struct drm_connector *connector)
 	return intel_dp_mst_get_ddc_modes(connector);
 }
 
+static int
+intel_dp_mst_mode_clock_exceeds_pbn_bw(struct drm_connector *connector,
+				       struct drm_modeset_acquire_ctx *ctx,
+				       int clock, int bpp)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_dp *intel_dp = intel_connector->mst_port;
+	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
+	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;
+	int ret = MODE_OK;
+
+	if (!mgr)
+		return ret;
+
+	ret = drm_modeset_lock(&mgr->base.lock, ctx);
+	if (ret == -EDEADLK)
+		return ret;
+
+	if (port->full_pbn &&
+	    drm_dp_calc_pbn_mode(clock, bpp, false) > port->full_pbn)
+		ret = MODE_CLOCK_HIGH;
+
+	return ret;
+}
+
 static enum drm_mode_status
-intel_dp_mst_mode_valid(struct drm_connector *connector,
-			struct drm_display_mode *mode)
+intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
+			    struct drm_display_mode *mode,
+			    struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct intel_dp *intel_dp = intel_connector->mst_port;
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
+	int ret;
 
 	if (drm_connector_is_unregistered(connector))
 		return MODE_ERROR;
@@ -632,7 +659,11 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
 	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
 	mode_rate = intel_dp_link_required(mode->clock, 18);
 
-	/* TODO - validate mode against available PBN for link */
+	ret = intel_dp_mst_mode_clock_exceeds_pbn_bw(connector, ctx,
+						     mode->clock, 24);
+	if (ret != MODE_OK)
+		return ret;
+
 	if (mode->clock < 10000)
 		return MODE_CLOCK_LOW;
 
@@ -671,7 +702,7 @@ intel_dp_mst_detect(struct drm_connector *connector,
 
 static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = {
 	.get_modes = intel_dp_mst_get_modes,
-	.mode_valid = intel_dp_mst_mode_valid,
+	.mode_valid_ctx = intel_dp_mst_mode_valid_ctx,
 	.atomic_best_encoder = intel_mst_atomic_best_encoder,
 	.atomic_check = intel_dp_mst_atomic_check,
 	.detect_ctx = intel_dp_mst_detect,
-- 
2.26.2

Re: [PATCH 1/2] drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

[Imre Deak]

Sorry for the delay, the review as I promised:

On Tue, May 26, 2020 at 02:23:09PM -0400, Lyude Paul wrote:
> This is just an atomic version of mode_valid, which is intended to be
> used for situations where a driver might need to check the atomic state
> of objects other than the connector itself. One such example is with
> MST, where the maximum possible bandwidth on a connector can change
> dynamically irregardless of the display configuration.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Lee Shawn C <shawn.c.lee@intel.com>
> Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper_internal.h |  6 +-
>  drivers/gpu/drm/drm_probe_helper.c         | 65 ++++++++++++++--------
>  include/drm/drm_modeset_helper_vtables.h   | 41 ++++++++++++++
>  3 files changed, 88 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index f0a66ef47e5ad..ca767cba6094d 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -73,8 +73,10 @@ enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>  					 const struct drm_display_mode *mode);
>  enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
>  					    const struct drm_display_mode *mode);
> -enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> -					      struct drm_display_mode *mode);
> +enum drm_mode_status
> +drm_connector_mode_valid(struct drm_connector *connector,
> +			 struct drm_display_mode *mode,
> +			 struct drm_modeset_acquire_ctx *ctx);
>  
>  struct drm_encoder *
>  drm_connector_get_single_encoder(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 466dfbba82564..3132784736841 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -86,16 +86,17 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
>  	return MODE_OK;
>  }
>  
> -static enum drm_mode_status
> +static int
>  drm_mode_validate_pipeline(struct drm_display_mode *mode,
> -			    struct drm_connector *connector)
> +			   struct drm_connector *connector,
> +			   struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_device *dev = connector->dev;
> -	enum drm_mode_status ret = MODE_OK;
>  	struct drm_encoder *encoder;
> +	int ret = MODE_OK;
>  
>  	/* Step 1: Validate against connector */
> -	ret = drm_connector_mode_valid(connector, mode);
> +	ret = drm_connector_mode_valid(connector, mode, ctx);
>  	if (ret != MODE_OK)
>  		return ret;
>  
> @@ -196,16 +197,23 @@ enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
>  	return encoder_funcs->mode_valid(encoder, mode);
>  }
>  
> -enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> -					      struct drm_display_mode *mode)
> +int
> +drm_connector_mode_valid(struct drm_connector *connector,
> +			 struct drm_display_mode *mode,
> +			 struct drm_modeset_acquire_ctx *ctx)
>  {
>  	const struct drm_connector_helper_funcs *connector_funcs =
>  		connector->helper_private;
>  
> -	if (!connector_funcs || !connector_funcs->mode_valid)
> +	if (!connector_funcs)
>  		return MODE_OK;
>  
> -	return connector_funcs->mode_valid(connector, mode);
> +	if (connector_funcs->mode_valid_ctx)
> +		return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> +	else if (connector_funcs->mode_valid)
> +		return connector_funcs->mode_valid(connector, mode);
> +	else
> +		return MODE_OK;
>  }
>  
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> @@ -375,8 +383,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
>   *      (if specified)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or sink specific checks
> + *    - the optional &drm_connector_helper_funcs.mode_valid or
> + *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform driver
> + *      and/or sink specific checks
>   *    - the optional &drm_crtc_helper_funcs.mode_valid,
>   *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
>   *      helpers can perform driver and/or source specific checks which are also
> @@ -507,22 +516,34 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
>  
>  	list_for_each_entry(mode, &connector->modes, head) {
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_pipeline(mode,
> -								  connector);
> +		ret = drm_mode_validate_pipeline(mode, connector, &ctx);
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;
> +		} else if (WARN_ON_ONCE(ret < 0)) {
> +			mode->status = MODE_BAD;
> +		} else {
> +			mode->status = ret;
> +		}
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_ycbcr420(mode,
> -								  connector);
> +		if (mode->status != MODE_OK)
> +			continue;
> +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
>  	}
>  
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f084631..8f020c3424b2b 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -968,6 +968,47 @@ struct drm_connector_helper_funcs {
>  	 */
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   struct drm_display_mode *mode);
> +
> +	/**
> +	 * @mode_valid_ctx:
> +	 *
> +	 * Callback to validate a mode for a connector, irrespective of the
> +	 * specific display configuration.
> +	 *
> +	 * This callback is used by the probe helpers to filter the mode list
> +	 * (which is usually derived from the EDID data block from the sink).
> +	 * See e.g. drm_helper_probe_single_connector_modes().
> +	 *
> +	 * This function is optional, and is the atomic version of
> +	 * &drm_connector_funcs.mode_valid.
> +	 *
> +	 * To allow for accessing the atomic state of modesetting objects, the
> +	 * helper libraries always call this with ctx set to a valid context,
> +	 * and &drm_mode_config.connection_mutex will always be locked with
> +	 * the ctx parameter set to @ctx. This allows for taking additional
> +	 * locks as required.
> +	 *
> +	 * Even though additional locks may be acquired, this callback is
> +	 * still expected not to take any constraints into account which would
> +	 * be influenced by the currently set display state - such constraints
> +	 * should be handled in the driver's atomic check. For example, if a
> +	 * connector shares display bandwidth with other connectors then it
> +	 * would be ok to validate a mode uses against the maximum possible
                                     ^mode against?

Reviewed-by: Imre Deak <imre.deak@intel.com>

> +	 * bandwidth of the connector. But it wouldn't be ok to take the
> +	 * current bandwidth usage of other connectors into account, as this
> +	 * would change depending on the display state.
> +	 *
> +	 * Returns:
> +	 *
> +	 * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> +	 * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> +	 * occurred and we need to backoff.
> +	 *
> +	 */
> +	int (*mode_valid_ctx)(struct drm_connector *connector,
> +			      struct drm_display_mode *mode,
> +			      struct drm_modeset_acquire_ctx *ctx);
> +
>  	/**
>  	 * @best_encoder:
>  	 *
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/mst: filter out the display mode exceed sink's capability

[Imre Deak]

On Tue, May 26, 2020 at 02:23:10PM -0400, Lyude Paul wrote:
> From: Lee Shawn C <shawn.c.lee@intel.com>
> 
> So far, max dot clock rate for MST mode rely on physcial
> bandwidth limitation. It would caused compatibility issue
> if source display resolution exceed MST hub output ability.
> 
> For example, source DUT had DP 1.2 output capability.
> And MST docking just support HDMI 1.4 spec. When a HDMI 2.0
> monitor connected. Source would retrieve EDID from external
> and get max resolution 4k@60fps. DP 1.2 can support 4K@60fps
> because it did not surpass DP physical bandwidth limitation.
> Do modeset to 4k@60fps, source output display data but MST
> docking can't output HDMI properly due to this resolution
> already over HDMI 1.4 spec.
> 
> Refer to commit <fcf463807596> ("drm/dp_mst: Use full_pbn
> instead of available_pbn for bandwidth checks").
> Source driver should refer to full_pbn to evaluate sink
> output capability. And filter out the resolution surpass
> sink output limitation.
> 
> v2: Using mgr->base.lock to protect full_pbn.
> v3: Add ctx lock.
> v4:
> * s/intel_dp_mst_mode_clock_exceed_pbn_bandwidth/
>   intel_dp_mst_mode_clock_exceeds_pbn_bw/
> * Use the new drm_connector_helper_funcs.mode_valid_ctx to properly pipe
>   down the drm_modeset_acquire_ctx that the probe helpers are using, so
>   we can safely grab &mgr->base.lock without deadlocking
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Co-developed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 39 ++++++++++++++++++---
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index d18b406f2a7d2..cf052095ad785 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -610,15 +610,42 @@ static int intel_dp_mst_get_modes(struct drm_connector *connector)
>  	return intel_dp_mst_get_ddc_modes(connector);
>  }
>  
> +static int
> +intel_dp_mst_mode_clock_exceeds_pbn_bw(struct drm_connector *connector,
> +				       struct drm_modeset_acquire_ctx *ctx,
> +				       int clock, int bpp)
> +{
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct intel_dp *intel_dp = intel_connector->mst_port;
> +	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
> +	struct drm_dp_mst_port *port = (to_intel_connector(connector))->port;

					intel_connector

> +	int ret = MODE_OK;
> +
> +	if (!mgr)

As a NULL check this would be bogus, but also connector->mst_port and so
mst_mgr too should be always non-NULL?

> +		return ret;
> +
> +	ret = drm_modeset_lock(&mgr->base.lock, ctx);
> +	if (ret == -EDEADLK)
> +		return ret;
> +
> +	if (port->full_pbn &&

How could full_pbn be unset?

> +	    drm_dp_calc_pbn_mode(clock, bpp, false) > port->full_pbn)
> +		ret = MODE_CLOCK_HIGH;
> +
> +	return ret;
> +}
> +
>  static enum drm_mode_status
> -intel_dp_mst_mode_valid(struct drm_connector *connector,
> -			struct drm_display_mode *mode)
> +intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> +			    struct drm_display_mode *mode,
> +			    struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_dp *intel_dp = intel_connector->mst_port;
>  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
> +	int ret;
>  
>  	if (drm_connector_is_unregistered(connector))
>  		return MODE_ERROR;
> @@ -632,7 +659,11 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
>  	mode_rate = intel_dp_link_required(mode->clock, 18);
>  
> -	/* TODO - validate mode against available PBN for link */
> +	ret = intel_dp_mst_mode_clock_exceeds_pbn_bw(connector, ctx,
> +						     mode->clock, 24);

Why 24 bpp and not 18?

Nit: could be checked after max_rate/max_dotclk for consistency.

> +	if (ret != MODE_OK)
> +		return ret;
> +
>  	if (mode->clock < 10000)
>  		return MODE_CLOCK_LOW;
>  
> @@ -671,7 +702,7 @@ intel_dp_mst_detect(struct drm_connector *connector,
>  
>  static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = {
>  	.get_modes = intel_dp_mst_get_modes,
> -	.mode_valid = intel_dp_mst_mode_valid,
> +	.mode_valid_ctx = intel_dp_mst_mode_valid_ctx,
>  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
>  	.atomic_check = intel_dp_mst_atomic_check,
>  	.detect_ctx = intel_dp_mst_detect,
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 1/2] drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

[Lyude Paul]

JFYI - found an issue with this patch that wouldn't have shown up on i915, info
down below: 

On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> Sorry for the delay, the review as I promised:
> 
> On Tue, May 26, 2020 at 02:23:09PM -0400, Lyude Paul wrote:
> > This is just an atomic version of mode_valid, which is intended to be
> > used for situations where a driver might need to check the atomic state
> > of objects other than the connector itself. One such example is with
> > MST, where the maximum possible bandwidth on a connector can change
> > dynamically irregardless of the display configuration.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Cc: Lee Shawn C <shawn.c.lee@intel.com>
> > Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc_helper_internal.h |  6 +-
> >  drivers/gpu/drm/drm_probe_helper.c         | 65 ++++++++++++++--------
> >  include/drm/drm_modeset_helper_vtables.h   | 41 ++++++++++++++
> >  3 files changed, 88 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > index f0a66ef47e5ad..ca767cba6094d 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > @@ -73,8 +73,10 @@ enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc
> > *crtc,
> >  					 const struct drm_display_mode *mode);
> >  enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
> >  					    const struct drm_display_mode
> > *mode);
> > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > *connector,
> > -					      struct drm_display_mode *mode);
> > +enum drm_mode_status
> > +drm_connector_mode_valid(struct drm_connector *connector,
> > +			 struct drm_display_mode *mode,
> > +			 struct drm_modeset_acquire_ctx *ctx);
> >  
> >  struct drm_encoder *
> >  drm_connector_get_single_encoder(struct drm_connector *connector);
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 466dfbba82564..3132784736841 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -86,16 +86,17 @@ drm_mode_validate_flag(const struct drm_display_mode
> > *mode,
> >  	return MODE_OK;
> >  }
> >  
> > -static enum drm_mode_status
> > +static int
> >  drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > -			    struct drm_connector *connector)
> > +			   struct drm_connector *connector,
> > +			   struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_device *dev = connector->dev;
> > -	enum drm_mode_status ret = MODE_OK;
> >  	struct drm_encoder *encoder;
> > +	int ret = MODE_OK;
> >  
> >  	/* Step 1: Validate against connector */
> > -	ret = drm_connector_mode_valid(connector, mode);
> > +	ret = drm_connector_mode_valid(connector, mode, ctx);
> >  	if (ret != MODE_OK)
> >  		return ret;
> >  
> > @@ -196,16 +197,23 @@ enum drm_mode_status drm_encoder_mode_valid(struct
> > drm_encoder *encoder,
> >  	return encoder_funcs->mode_valid(encoder, mode);
> >  }
> >  
> > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > *connector,
> > -					      struct drm_display_mode *mode)
> > +int
> > +drm_connector_mode_valid(struct drm_connector *connector,
> > +			 struct drm_display_mode *mode,
> > +			 struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	const struct drm_connector_helper_funcs *connector_funcs =
> >  		connector->helper_private;
> >  
> > -	if (!connector_funcs || !connector_funcs->mode_valid)
> > +	if (!connector_funcs)
> >  		return MODE_OK;
> >  
> > -	return connector_funcs->mode_valid(connector, mode);
> > +	if (connector_funcs->mode_valid_ctx)
> > +		return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> > +	else if (connector_funcs->mode_valid)
> > +		return connector_funcs->mode_valid(connector, mode);
> > +	else
> > +		return MODE_OK;
> >  }
> >  
> >  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > @@ -375,8 +383,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
> >   *      (if specified)
> >   *    - drm_mode_validate_flag() checks the modes against basic connector
> >   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> > - *    - the optional &drm_connector_helper_funcs.mode_valid helper can
> > perform
> > - *      driver and/or sink specific checks
> > + *    - the optional &drm_connector_helper_funcs.mode_valid or
> > + *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform
> > driver
> > + *      and/or sink specific checks
> >   *    - the optional &drm_crtc_helper_funcs.mode_valid,
> >   *      &drm_bridge_funcs.mode_valid and
> > &drm_encoder_helper_funcs.mode_valid
> >   *      helpers can perform driver and/or source specific checks which are
> > also
> > @@ -507,22 +516,34 @@ int drm_helper_probe_single_connector_modes(struct
> > drm_connector *connector,
> >  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> >  
> >  	list_for_each_entry(mode, &connector->modes, head) {
> > -		if (mode->status == MODE_OK)
> > -			mode->status = drm_mode_validate_driver(dev, mode);
> > +		if (mode->status != MODE_OK)
> > +			continue;
> > +
> > +		mode->status = drm_mode_validate_driver(dev, mode);
> > +		if (mode->status != MODE_OK)
> > +			continue;
> >  
> > -		if (mode->status == MODE_OK)
> > -			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > +		if (mode->status != MODE_OK)
> > +			continue;
> >  
> > -		if (mode->status == MODE_OK)
> > -			mode->status = drm_mode_validate_flag(mode, mode_flags);
> > +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> > +		if (mode->status != MODE_OK)
> > +			continue;
> >  
> > -		if (mode->status == MODE_OK)
> > -			mode->status = drm_mode_validate_pipeline(mode,
> > -								  connector);
> > +		ret = drm_mode_validate_pipeline(mode, connector, &ctx);
> > +		if (ret == -EDEADLK) {
> > +			drm_modeset_backoff(&ctx);
> > +			goto retry;
> > +		} else if (WARN_ON_ONCE(ret < 0)) {
> > +			mode->status = MODE_BAD;

This check is wrong actually. We define negative values for drm_mode_status
(MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers currently
seem to use them, are something we want to treat as not-unexpected errors and
not WARN_ON.

This is a bit annoying because it does mean there's some overlap between
drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH). Luckily
I can't see any reason why drivers would want to return those, but I think we
should probably print a debugging message when we get any errno values just so
developers don't get confused (also going to add a IS_ERR() equivalent for
drm_mode_status, but with a different name)

> > +		} else {
> > +			mode->status = ret;
> > +		}
> >  
> > -		if (mode->status == MODE_OK)
> > -			mode->status = drm_mode_validate_ycbcr420(mode,
> > -								  connector);
> > +		if (mode->status != MODE_OK)
> > +			continue;
> > +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
> >  	}
> >  
> >  prune:
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 421a30f084631..8f020c3424b2b 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -968,6 +968,47 @@ struct drm_connector_helper_funcs {
> >  	 */
> >  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> >  					   struct drm_display_mode *mode);
> > +
> > +	/**
> > +	 * @mode_valid_ctx:
> > +	 *
> > +	 * Callback to validate a mode for a connector, irrespective of the
> > +	 * specific display configuration.
> > +	 *
> > +	 * This callback is used by the probe helpers to filter the mode list
> > +	 * (which is usually derived from the EDID data block from the sink).
> > +	 * See e.g. drm_helper_probe_single_connector_modes().
> > +	 *
> > +	 * This function is optional, and is the atomic version of
> > +	 * &drm_connector_funcs.mode_valid.
> > +	 *
> > +	 * To allow for accessing the atomic state of modesetting objects, the
> > +	 * helper libraries always call this with ctx set to a valid context,
> > +	 * and &drm_mode_config.connection_mutex will always be locked with
> > +	 * the ctx parameter set to @ctx. This allows for taking additional
> > +	 * locks as required.
> > +	 *
> > +	 * Even though additional locks may be acquired, this callback is
> > +	 * still expected not to take any constraints into account which would
> > +	 * be influenced by the currently set display state - such constraints
> > +	 * should be handled in the driver's atomic check. For example, if a
> > +	 * connector shares display bandwidth with other connectors then it
> > +	 * would be ok to validate a mode uses against the maximum possible
>                                      ^mode against?
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > +	 * bandwidth of the connector. But it wouldn't be ok to take the
> > +	 * current bandwidth usage of other connectors into account, as this
> > +	 * would change depending on the display state.
> > +	 *
> > +	 * Returns:
> > +	 *
> > +	 * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> > +	 * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > +	 * occurred and we need to backoff.
> > +	 *
> > +	 */
> > +	int (*mode_valid_ctx)(struct drm_connector *connector,
> > +			      struct drm_display_mode *mode,
> > +			      struct drm_modeset_acquire_ctx *ctx);
> > +
> >  	/**
> >  	 * @best_encoder:
> >  	 *
> > -- 
> > 2.26.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

[Imre Deak]

On Wed, Jul 08, 2020 at 01:25:14PM -0400, Lyude Paul wrote:
> 
> JFYI - found an issue with this patch that wouldn't have shown up on i915, info
> down below: 
> 
> On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> > Sorry for the delay, the review as I promised:
> > 
> > On Tue, May 26, 2020 at 02:23:09PM -0400, Lyude Paul wrote:
> > > This is just an atomic version of mode_valid, which is intended to be
> > > used for situations where a driver might need to check the atomic state
> > > of objects other than the connector itself. One such example is with
> > > MST, where the maximum possible bandwidth on a connector can change
> > > dynamically irregardless of the display configuration.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Cc: Lee Shawn C <shawn.c.lee@intel.com>
> > > Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc_helper_internal.h |  6 +-
> > >  drivers/gpu/drm/drm_probe_helper.c         | 65 ++++++++++++++--------
> > >  include/drm/drm_modeset_helper_vtables.h   | 41 ++++++++++++++
> > >  3 files changed, 88 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > index f0a66ef47e5ad..ca767cba6094d 100644
> > > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > @@ -73,8 +73,10 @@ enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc
> > > *crtc,
> > >  					 const struct drm_display_mode *mode);
> > >  enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
> > >  					    const struct drm_display_mode
> > > *mode);
> > > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > > *connector,
> > > -					      struct drm_display_mode *mode);
> > > +enum drm_mode_status
> > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > +			 struct drm_display_mode *mode,
> > > +			 struct drm_modeset_acquire_ctx *ctx);
> > >  
> > >  struct drm_encoder *
> > >  drm_connector_get_single_encoder(struct drm_connector *connector);
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > b/drivers/gpu/drm/drm_probe_helper.c
> > > index 466dfbba82564..3132784736841 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -86,16 +86,17 @@ drm_mode_validate_flag(const struct drm_display_mode
> > > *mode,
> > >  	return MODE_OK;
> > >  }
> > >  
> > > -static enum drm_mode_status
> > > +static int
> > >  drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > > -			    struct drm_connector *connector)
> > > +			   struct drm_connector *connector,
> > > +			   struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct drm_device *dev = connector->dev;
> > > -	enum drm_mode_status ret = MODE_OK;
> > >  	struct drm_encoder *encoder;
> > > +	int ret = MODE_OK;
> > >  
> > >  	/* Step 1: Validate against connector */
> > > -	ret = drm_connector_mode_valid(connector, mode);
> > > +	ret = drm_connector_mode_valid(connector, mode, ctx);
> > >  	if (ret != MODE_OK)
> > >  		return ret;
> > >  
> > > @@ -196,16 +197,23 @@ enum drm_mode_status drm_encoder_mode_valid(struct
> > > drm_encoder *encoder,
> > >  	return encoder_funcs->mode_valid(encoder, mode);
> > >  }
> > >  
> > > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > > *connector,
> > > -					      struct drm_display_mode *mode)
> > > +int
> > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > +			 struct drm_display_mode *mode,
> > > +			 struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	const struct drm_connector_helper_funcs *connector_funcs =
> > >  		connector->helper_private;
> > >  
> > > -	if (!connector_funcs || !connector_funcs->mode_valid)
> > > +	if (!connector_funcs)
> > >  		return MODE_OK;
> > >  
> > > -	return connector_funcs->mode_valid(connector, mode);
> > > +	if (connector_funcs->mode_valid_ctx)
> > > +		return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> > > +	else if (connector_funcs->mode_valid)
> > > +		return connector_funcs->mode_valid(connector, mode);
> > > +	else
> > > +		return MODE_OK;
> > >  }
> > >  
> > >  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > > @@ -375,8 +383,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
> > >   *      (if specified)
> > >   *    - drm_mode_validate_flag() checks the modes against basic connector
> > >   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> > > - *    - the optional &drm_connector_helper_funcs.mode_valid helper can
> > > perform
> > > - *      driver and/or sink specific checks
> > > + *    - the optional &drm_connector_helper_funcs.mode_valid or
> > > + *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform
> > > driver
> > > + *      and/or sink specific checks
> > >   *    - the optional &drm_crtc_helper_funcs.mode_valid,
> > >   *      &drm_bridge_funcs.mode_valid and
> > > &drm_encoder_helper_funcs.mode_valid
> > >   *      helpers can perform driver and/or source specific checks which are
> > > also
> > > @@ -507,22 +516,34 @@ int drm_helper_probe_single_connector_modes(struct
> > > drm_connector *connector,
> > >  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> > >  
> > >  	list_for_each_entry(mode, &connector->modes, head) {
> > > -		if (mode->status == MODE_OK)
> > > -			mode->status = drm_mode_validate_driver(dev, mode);
> > > +		if (mode->status != MODE_OK)
> > > +			continue;
> > > +
> > > +		mode->status = drm_mode_validate_driver(dev, mode);
> > > +		if (mode->status != MODE_OK)
> > > +			continue;
> > >  
> > > -		if (mode->status == MODE_OK)
> > > -			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > > +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > > +		if (mode->status != MODE_OK)
> > > +			continue;
> > >  
> > > -		if (mode->status == MODE_OK)
> > > -			mode->status = drm_mode_validate_flag(mode, mode_flags);
> > > +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> > > +		if (mode->status != MODE_OK)
> > > +			continue;
> > >  
> > > -		if (mode->status == MODE_OK)
> > > -			mode->status = drm_mode_validate_pipeline(mode,
> > > -								  connector);
> > > +		ret = drm_mode_validate_pipeline(mode, connector, &ctx);
> > > +		if (ret == -EDEADLK) {
> > > +			drm_modeset_backoff(&ctx);
> > > +			goto retry;
> > > +		} else if (WARN_ON_ONCE(ret < 0)) {
> > > +			mode->status = MODE_BAD;
> 
> This check is wrong actually. We define negative values for drm_mode_status
> (MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers currently
> seem to use them, are something we want to treat as not-unexpected errors and
> not WARN_ON.

Ah, yes, missed that.

> This is a bit annoying because it does mean there's some overlap between
> drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH). Luckily
> I can't see any reason why drivers would want to return those, but I think we
> should probably print a debugging message when we get any errno values just so
> developers don't get confused (also going to add a IS_ERR() equivalent for
> drm_mode_status, but with a different name)

Maybe I'm missing something, but having negative values in
drm_mode_status doesn't make sense to me, as all the non-zero values
there just mean the mode is incorrect for some reason and should be
pruned.

Would it make sense to better separate the mode-status value and any
error return value from mode_valid_ctx(), by adding a drm_mode_status *
parameter to the hook and drm_mode_validate_pipeline()?

> 
> > > +		} else {
> > > +			mode->status = ret;
> > > +		}
> > >  
> > > -		if (mode->status == MODE_OK)
> > > -			mode->status = drm_mode_validate_ycbcr420(mode,
> > > -								  connector);
> > > +		if (mode->status != MODE_OK)
> > > +			continue;
> > > +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
> > >  	}
> > >  
> > >  prune:
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > b/include/drm/drm_modeset_helper_vtables.h
> > > index 421a30f084631..8f020c3424b2b 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -968,6 +968,47 @@ struct drm_connector_helper_funcs {
> > >  	 */
> > >  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> > >  					   struct drm_display_mode *mode);
> > > +
> > > +	/**
> > > +	 * @mode_valid_ctx:
> > > +	 *
> > > +	 * Callback to validate a mode for a connector, irrespective of the
> > > +	 * specific display configuration.
> > > +	 *
> > > +	 * This callback is used by the probe helpers to filter the mode list
> > > +	 * (which is usually derived from the EDID data block from the sink).
> > > +	 * See e.g. drm_helper_probe_single_connector_modes().
> > > +	 *
> > > +	 * This function is optional, and is the atomic version of
> > > +	 * &drm_connector_funcs.mode_valid.
> > > +	 *
> > > +	 * To allow for accessing the atomic state of modesetting objects, the
> > > +	 * helper libraries always call this with ctx set to a valid context,
> > > +	 * and &drm_mode_config.connection_mutex will always be locked with
> > > +	 * the ctx parameter set to @ctx. This allows for taking additional
> > > +	 * locks as required.
> > > +	 *
> > > +	 * Even though additional locks may be acquired, this callback is
> > > +	 * still expected not to take any constraints into account which would
> > > +	 * be influenced by the currently set display state - such constraints
> > > +	 * should be handled in the driver's atomic check. For example, if a
> > > +	 * connector shares display bandwidth with other connectors then it
> > > +	 * would be ok to validate a mode uses against the maximum possible
> >                                      ^mode against?
> > 
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > > +	 * bandwidth of the connector. But it wouldn't be ok to take the
> > > +	 * current bandwidth usage of other connectors into account, as this
> > > +	 * would change depending on the display state.
> > > +	 *
> > > +	 * Returns:
> > > +	 *
> > > +	 * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> > > +	 * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > > +	 * occurred and we need to backoff.
> > > +	 *
> > > +	 */
> > > +	int (*mode_valid_ctx)(struct drm_connector *connector,
> > > +			      struct drm_display_mode *mode,
> > > +			      struct drm_modeset_acquire_ctx *ctx);
> > > +
> > >  	/**
> > >  	 * @best_encoder:
> > >  	 *
> > > -- 
> > > 2.26.2
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/2] drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx

[Lyude Paul]

On Wed, 2020-07-08 at 21:45 +0300, Imre Deak wrote:
> On Wed, Jul 08, 2020 at 01:25:14PM -0400, Lyude Paul wrote:
> > JFYI - found an issue with this patch that wouldn't have shown up on i915,
> > info
> > down below: 
> > 
> > On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> > > Sorry for the delay, the review as I promised:
> > > 
> > > On Tue, May 26, 2020 at 02:23:09PM -0400, Lyude Paul wrote:
> > > > This is just an atomic version of mode_valid, which is intended to be
> > > > used for situations where a driver might need to check the atomic state
> > > > of objects other than the connector itself. One such example is with
> > > > MST, where the maximum possible bandwidth on a connector can change
> > > > dynamically irregardless of the display configuration.
> > > > 
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Cc: Lee Shawn C <shawn.c.lee@intel.com>
> > > > Tested-by: Lee Shawn C <shawn.c.lee@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_crtc_helper_internal.h |  6 +-
> > > >  drivers/gpu/drm/drm_probe_helper.c         | 65 ++++++++++++++--------
> > > >  include/drm/drm_modeset_helper_vtables.h   | 41 ++++++++++++++
> > > >  3 files changed, 88 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > index f0a66ef47e5ad..ca767cba6094d 100644
> > > > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > @@ -73,8 +73,10 @@ enum drm_mode_status drm_crtc_mode_valid(struct
> > > > drm_crtc
> > > > *crtc,
> > > >  					 const struct drm_display_mode
> > > > *mode);
> > > >  enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder
> > > > *encoder,
> > > >  					    const struct
> > > > drm_display_mode
> > > > *mode);
> > > > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > > > *connector,
> > > > -					      struct drm_display_mode
> > > > *mode);
> > > > +enum drm_mode_status
> > > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > > +			 struct drm_display_mode *mode,
> > > > +			 struct drm_modeset_acquire_ctx *ctx);
> > > >  
> > > >  struct drm_encoder *
> > > >  drm_connector_get_single_encoder(struct drm_connector *connector);
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > index 466dfbba82564..3132784736841 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -86,16 +86,17 @@ drm_mode_validate_flag(const struct drm_display_mode
> > > > *mode,
> > > >  	return MODE_OK;
> > > >  }
> > > >  
> > > > -static enum drm_mode_status
> > > > +static int
> > > >  drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > > > -			    struct drm_connector *connector)
> > > > +			   struct drm_connector *connector,
> > > > +			   struct drm_modeset_acquire_ctx *ctx)
> > > >  {
> > > >  	struct drm_device *dev = connector->dev;
> > > > -	enum drm_mode_status ret = MODE_OK;
> > > >  	struct drm_encoder *encoder;
> > > > +	int ret = MODE_OK;
> > > >  
> > > >  	/* Step 1: Validate against connector */
> > > > -	ret = drm_connector_mode_valid(connector, mode);
> > > > +	ret = drm_connector_mode_valid(connector, mode, ctx);
> > > >  	if (ret != MODE_OK)
> > > >  		return ret;
> > > >  
> > > > @@ -196,16 +197,23 @@ enum drm_mode_status drm_encoder_mode_valid(struct
> > > > drm_encoder *encoder,
> > > >  	return encoder_funcs->mode_valid(encoder, mode);
> > > >  }
> > > >  
> > > > -enum drm_mode_status drm_connector_mode_valid(struct drm_connector
> > > > *connector,
> > > > -					      struct drm_display_mode
> > > > *mode)
> > > > +int
> > > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > > +			 struct drm_display_mode *mode,
> > > > +			 struct drm_modeset_acquire_ctx *ctx)
> > > >  {
> > > >  	const struct drm_connector_helper_funcs *connector_funcs =
> > > >  		connector->helper_private;
> > > >  
> > > > -	if (!connector_funcs || !connector_funcs->mode_valid)
> > > > +	if (!connector_funcs)
> > > >  		return MODE_OK;
> > > >  
> > > > -	return connector_funcs->mode_valid(connector, mode);
> > > > +	if (connector_funcs->mode_valid_ctx)
> > > > +		return connector_funcs->mode_valid_ctx(connector, mode,
> > > > ctx);
> > > > +	else if (connector_funcs->mode_valid)
> > > > +		return connector_funcs->mode_valid(connector, mode);
> > > > +	else
> > > > +		return MODE_OK;
> > > >  }
> > > >  
> > > >  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > > > @@ -375,8 +383,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
> > > >   *      (if specified)
> > > >   *    - drm_mode_validate_flag() checks the modes against basic
> > > > connector
> > > >   *      capabilities
> > > > (interlace_allowed,doublescan_allowed,stereo_allowed)
> > > > - *    - the optional &drm_connector_helper_funcs.mode_valid helper can
> > > > perform
> > > > - *      driver and/or sink specific checks
> > > > + *    - the optional &drm_connector_helper_funcs.mode_valid or
> > > > + *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform
> > > > driver
> > > > + *      and/or sink specific checks
> > > >   *    - the optional &drm_crtc_helper_funcs.mode_valid,
> > > >   *      &drm_bridge_funcs.mode_valid and
> > > > &drm_encoder_helper_funcs.mode_valid
> > > >   *      helpers can perform driver and/or source specific checks which
> > > > are
> > > > also
> > > > @@ -507,22 +516,34 @@ int drm_helper_probe_single_connector_modes(struct
> > > > drm_connector *connector,
> > > >  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
> > > >  
> > > >  	list_for_each_entry(mode, &connector->modes, head) {
> > > > -		if (mode->status == MODE_OK)
> > > > -			mode->status = drm_mode_validate_driver(dev,
> > > > mode);
> > > > +		if (mode->status != MODE_OK)
> > > > +			continue;
> > > > +
> > > > +		mode->status = drm_mode_validate_driver(dev, mode);
> > > > +		if (mode->status != MODE_OK)
> > > > +			continue;
> > > >  
> > > > -		if (mode->status == MODE_OK)
> > > > -			mode->status = drm_mode_validate_size(mode,
> > > > maxX, maxY);
> > > > +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> > > > +		if (mode->status != MODE_OK)
> > > > +			continue;
> > > >  
> > > > -		if (mode->status == MODE_OK)
> > > > -			mode->status = drm_mode_validate_flag(mode,
> > > > mode_flags);
> > > > +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> > > > +		if (mode->status != MODE_OK)
> > > > +			continue;
> > > >  
> > > > -		if (mode->status == MODE_OK)
> > > > -			mode->status = drm_mode_validate_pipeline(mode,
> > > > -								  connec
> > > > tor);
> > > > +		ret = drm_mode_validate_pipeline(mode, connector, &ctx);
> > > > +		if (ret == -EDEADLK) {
> > > > +			drm_modeset_backoff(&ctx);
> > > > +			goto retry;
> > > > +		} else if (WARN_ON_ONCE(ret < 0)) {
> > > > +			mode->status = MODE_BAD;
> > 
> > This check is wrong actually. We define negative values for drm_mode_status
> > (MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers
> > currently
> > seem to use them, are something we want to treat as not-unexpected errors
> > and
> > not WARN_ON.
> 
> Ah, yes, missed that.
> 
> > This is a bit annoying because it does mean there's some overlap between
> > drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH).
> > Luckily
> > I can't see any reason why drivers would want to return those, but I think
> > we
> > should probably print a debugging message when we get any errno values just
> > so
> > developers don't get confused (also going to add a IS_ERR() equivalent for
> > drm_mode_status, but with a different name)
> 
> Maybe I'm missing something, but having negative values in
> drm_mode_status doesn't make sense to me, as all the non-zero values
> there just mean the mode is incorrect for some reason and should be
> pruned.
> 
> Would it make sense to better separate the mode-status value and any
> error return value from mode_valid_ctx(), by adding a drm_mode_status *
> parameter to the hook and drm_mode_validate_pipeline()?

yeah-that would probably make more sense. I will go ahead and do that
> 
> > > > +		} else {
> > > > +			mode->status = ret;
> > > > +		}
> > > >  
> > > > -		if (mode->status == MODE_OK)
> > > > -			mode->status = drm_mode_validate_ycbcr420(mode,
> > > > -								  connec
> > > > tor);
> > > > +		if (mode->status != MODE_OK)
> > > > +			continue;
> > > > +		mode->status = drm_mode_validate_ycbcr420(mode,
> > > > connector);
> > > >  	}
> > > >  
> > > >  prune:
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > index 421a30f084631..8f020c3424b2b 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -968,6 +968,47 @@ struct drm_connector_helper_funcs {
> > > >  	 */
> > > >  	enum drm_mode_status (*mode_valid)(struct drm_connector
> > > > *connector,
> > > >  					   struct drm_display_mode
> > > > *mode);
> > > > +
> > > > +	/**
> > > > +	 * @mode_valid_ctx:
> > > > +	 *
> > > > +	 * Callback to validate a mode for a connector, irrespective of
> > > > the
> > > > +	 * specific display configuration.
> > > > +	 *
> > > > +	 * This callback is used by the probe helpers to filter the mode
> > > > list
> > > > +	 * (which is usually derived from the EDID data block from the
> > > > sink).
> > > > +	 * See e.g. drm_helper_probe_single_connector_modes().
> > > > +	 *
> > > > +	 * This function is optional, and is the atomic version of
> > > > +	 * &drm_connector_funcs.mode_valid.
> > > > +	 *
> > > > +	 * To allow for accessing the atomic state of modesetting
> > > > objects, the
> > > > +	 * helper libraries always call this with ctx set to a valid
> > > > context,
> > > > +	 * and &drm_mode_config.connection_mutex will always be locked
> > > > with
> > > > +	 * the ctx parameter set to @ctx. This allows for taking
> > > > additional
> > > > +	 * locks as required.
> > > > +	 *
> > > > +	 * Even though additional locks may be acquired, this callback
> > > > is
> > > > +	 * still expected not to take any constraints into account which
> > > > would
> > > > +	 * be influenced by the currently set display state - such
> > > > constraints
> > > > +	 * should be handled in the driver's atomic check. For example,
> > > > if a
> > > > +	 * connector shares display bandwidth with other connectors then
> > > > it
> > > > +	 * would be ok to validate a mode uses against the maximum
> > > > possible
> > >                                      ^mode against?
> > > 
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > > +	 * bandwidth of the connector. But it wouldn't be ok to take the
> > > > +	 * current bandwidth usage of other connectors into account, as
> > > > this
> > > > +	 * would change depending on the display state.
> > > > +	 *
> > > > +	 * Returns:
> > > > +	 *
> > > > +	 * Either &drm_mode_status.MODE_OK, one of the failure reasons
> > > > in
> > > > +	 * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > > > +	 * occurred and we need to backoff.
> > > > +	 *
> > > > +	 */
> > > > +	int (*mode_valid_ctx)(struct drm_connector *connector,
> > > > +			      struct drm_display_mode *mode,
> > > > +			      struct drm_modeset_acquire_ctx *ctx);
> > > > +
> > > >  	/**
> > > >  	 * @best_encoder:
> > > >  	 *
> > > > -- 
> > > > 2.26.2
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel