Re: [Intel-gfx] [PATCH] drm/i915: Check require bandwidth did not exceed LSPCON limitation

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 17 Jan 2020, Lee Shawn C <shawn.c.lee@intel.com> wrote:
> While mode setting, driver would calculate mode rate based on
> resolution and bpp. And choose the best bpp that did not exceed
> DP bandwidtd.
>
> But LSPCON had more restriction due to it convert DP to HDMI.
> Driver should respect HDMI's bandwidth limitation if LSPCON
> was active. This change would ignore the bpp when its required
> output bandwidth already over HDMI 2.0 or 1.4 spec.
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Sam McNally <sammc@google.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 45 +++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_lspcon.c |  5 +++
>  drivers/gpu/drm/i915/display/intel_lspcon.h |  1 +
>  3 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index c7424e2a04a3..c27d3e7ac219 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1976,6 +1976,47 @@ static int intel_dp_output_bpp(const struct intel_crtc_state *crtc_state, int bp
>  	return bpp;
>  }
>  
> +static bool
> +intel_dp_lspcon_exceed_bandwidth_limitation(struct intel_dp *intel_dp,
> +					    struct intel_crtc_state *pipe_config,
> +					    int bpp)
> +{
> +	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	const struct drm_display_info *info = &connector->base.display_info;
> +	enum drm_lspcon_mode lspcon_current_mode = lspcon_get_mode(lspcon);
> +	const int pcon_mode_max_tmds_clock = 600000;
> +	const int ls_mode_max_tmds_clock   = 340000;
> +	int mode_rate, max_tmds_clock = pcon_mode_max_tmds_clock;
> +
> +	if (lspcon->active) {
> +		switch (bpp) {
> +		case 36:
> +			mode_rate = pipe_config->hw.adjusted_mode.crtc_clock * 3 / 2;
> +			break;
> +		case 30:
> +			mode_rate = pipe_config->hw.adjusted_mode.crtc_clock * 5 / 4;
> +			break;
> +		case 24:
> +		default:
> +			mode_rate = pipe_config->hw.adjusted_mode.crtc_clock;
> +			break;
> +		}
> +
> +		if (lspcon_current_mode == DRM_LSPCON_MODE_LS)
> +			max_tmds_clock = ls_mode_max_tmds_clock;
> +
> +		if (info->max_tmds_clock)
> +			max_tmds_clock = min(max_tmds_clock,
> +					     info->max_tmds_clock);
> +
> +		if (mode_rate > max_tmds_clock)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +

Instead of this, please add a simple intel_lspcon.c function:

	int lspcon_max_rate(struct intel_lspcon *lspcon);

that returns the max rate. Everything else can then be done in
intel_dp.c around this. The function gets simplified enough that you can
throw out the const ints and use the values directly.

>  /* Optimize link config in order: max bpp, min clock, min lanes */
>  static int
>  intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> @@ -1989,6 +2030,10 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>  	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
>  		int output_bpp = intel_dp_output_bpp(pipe_config, bpp);
>  
> +		/* Bypass th bpp if require bandwidth over HDMI spec when LSPCON active */
> +		if (intel_dp_lspcon_exceed_bandwidth_limitation(intel_dp, pipe_config, output_bpp))
> +			continue;
> +

The placing sticks out like a sore thumb. I think we need to filter out
the modes already in intel_dp_mode_valid(). This isn't all that
different from intel_dp_downstream_max_dotclock() is it?

>  		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>  						   output_bpp);

But I guess since the mode valid limit is for 8 bpc, you'll also need a
check here? Maybe Ville has better ideas.

Here it would be something like:

	if (lspcon->active && mode_rate > lspcon_max_rate(lscon))
        	continue;

BR,
Jani.

>  
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
> index d807c5648c87..6952c5028fdf 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> @@ -550,6 +550,11 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon)
>  	lspcon_wait_mode(lspcon, DRM_LSPCON_MODE_PCON);
>  }
>  
> +int lspcon_get_mode(struct intel_lspcon *lspcon)
> +{
> +	return lspcon_get_current_mode(lspcon);
> +}
> +
>  bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  {
>  	struct intel_dp *dp = &intel_dig_port->dp;
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h
> index 37cfddf8a9c5..5ce9daef9708 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
> @@ -18,6 +18,7 @@ struct intel_lspcon;
>  bool lspcon_init(struct intel_digital_port *intel_dig_port);
>  void lspcon_resume(struct intel_lspcon *lspcon);
>  void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
> +int lspcon_get_mode(struct intel_lspcon *lspcon);
>  void lspcon_write_infoframe(struct intel_encoder *encoder,
>  			    const struct intel_crtc_state *crtc_state,
>  			    unsigned int type,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx