[Intel-gfx] [PATCH] drm/i915/dp/mst : Get clock rate from sink's available PBN

[Lyude Paul]

On Tue, 2020-01-07 at 01:41 +0800, Lee Shawn C wrote:
> Driver report physcial bandwidth for max dot clock rate.
> It would caused compatibility issue sometimes when physical
> bandwidth exceed MST hub output ability.
> 
> For example, here is a MST hub with HDMI 1.4 and DP 1.2 output.
> And source have DP 1.2 output capability. Connect a HDMI 2.0
> display then source will retrieve EDID from external monitor.
> Driver got max resolution was 4k at 60fps. DP 1.2 can support
> this resolution because it did not surpass max physical bandwidth.
> After modeset, source output display data but MST hub can't
> output HDMI properly due to it already over HDMI 1.4 spec.
> 
> Apply this calculation, source calcualte max dot clock according
> to available PBN. Driver will remove the mode that over current
> clock rate. And external display can works normally.
> 
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou at intel.com>
> 
> Signed-off-by: Lee Shawn C <shawn.c.lee at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 27 ++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 3b066c63816d..eaa440165ad2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -550,6 +550,27 @@ static int intel_dp_mst_get_modes(struct drm_connector
> *connector)
>  	return intel_dp_mst_get_ddc_modes(connector);
>  }
>  
> +static int
> +intel_dp_mst_downstream_max_dotclock(struct drm_connector *connector)
> +{
> +	struct intel_connector *intel_connector =
> to_intel_connector(connector);
> +	struct intel_dp *intel_dp = intel_connector->mst_port;
> +	struct drm_dp_mst_port *port;
> +	u64 clock_rate = 0;
> +
> +	if (intel_dp->mst_mgr.mst_primary)
> +		list_for_each_entry(port, &intel_dp->mst_mgr.mst_primary-
> >ports, next)
> +			if (port->connector == connector) {
> +				clock_rate = ((u64)port->available_pbn * (54 *
> 8 * 1000 * 1000)) / (64 * 1006);
> +
> +				// FIXME: We should used pipe bpp to do this
> calculation.
> +				//        But can't retrieve bpp setting from
> drm_connector.
> +				return (int)(clock_rate / 24);
> +			}
> +
> +	return to_i915(connector->dev)->max_dotclk_freq;
> +}

Hi! So-there's no need to loop through the ports like this, just use the
drm_dp_mst_port struct that's associated with intel_connector->port directly
(feel free to change the declaration to `struct drm_dp_mst_port *port` instead
of `void *port`, it's not illegal to dereference it anymore I promise!

Additionally - you don't want to use pipe_bpp here at all. My advice is to use
the hard-coded bpc we currently have for MST. Once you guys have retry logic
to dynamically select the bpc depending on the available bandwidth, I'd move
this check over to using the smallest possible BPC reported by the connector's
display_info. Remember we're checking if -any- variant of each mode is somehow
possible, it's ok and expected for modes to potentially fail at higher BPCs.

Anyway-this looks fine otherwise, but like Ville mentioned available_pbn isn't
the thing that we want to be using here. I've got support for using full_pbn
instead and that should be pushed sometime today or tommorrow (dealing with
some topic branch weirdness with dim right now). This is the patch series,
jfyi:

https://patchwork.freedesktop.org/series/74295/

Also-feel free to write a drm helper to do these mode_valid checks for mst, if
it's feasible and not overkill

> +
>  static enum drm_mode_status
>  intel_dp_mst_mode_valid(struct drm_connector *connector,
>  			struct drm_display_mode *mode)
> @@ -557,8 +578,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>  	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 max_rate, mode_rate, max_lanes, max_link_clock, max_dotclk;
>  
>  	if (drm_connector_is_unregistered(connector))
>  		return MODE_ERROR;
> @@ -572,7 +592,8 @@ 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 */
> +	max_dotclk = intel_dp_mst_downstream_max_dotclock(connector);
> +
>  	if (mode->clock < 10000)
>  		return MODE_CLOCK_LOW;
>  
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat