Re: [PATCH 2/2] drm/i915/dp: Validate mode against max. link data rate for DP MST

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 09, 2016 at 09:32:30PM -0800, Dhinakaran Pandiyan wrote:
> Not validating the the mode rate against link rate results not pruning
> invalid modes. For e.g, HBR2 5.4 Gpbs 2 lane configuration does not
> support 4k @ 60Hz. But, we do not reject this mode currently.
> 
> So, make use of the helpers in intel_dp in validate mode rates against
> max. data rate of a configuration.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c     |  4 ++--
>  drivers/gpu/drm/i915/intel_dp_mst.c | 12 +++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7a9e122..7a73e43 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -161,14 +161,14 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
>  	return min(source_max, sink_max);
>  }
>  
> -static int
> +int
>  intel_dp_link_required(int pixel_clock, int bpp)
>  {
>  	/* pixel_clock is in kHz, divide bpp by 8 to return the value in kBps*/
>  	return (pixel_clock * bpp + 7) / 8;
>  }
>  
> -static int
> +int
>  intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>  {
>  	/* max_link_clock is the link symbol clock (LS_Clk) in kHz and not the
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..38d2ce0 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -335,7 +335,17 @@ static enum drm_mode_status
>  intel_dp_mst_mode_valid(struct drm_connector *connector,
>  			struct drm_display_mode *mode)
>  {
> +	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 link_clock = intel_dp_max_link_rate(intel_dp);
> +	int lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> +	int bpp = 24; /* MST uses fixed bpp */
> +	int mode_rate;
> +	int link_max_data_rate;
> +
> +	link_max_data_rate = intel_dp_max_data_rate(link_clock, lane_count);
> +	mode_rate = intel_dp_link_required(mode->clock, bpp);
>  
>  	/* TODO - validate mode against available PBN for link */
>  	if (mode->clock < 10000)
> @@ -344,7 +354,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
>  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		return MODE_H_ILLEGAL;
>  
> -	if (mode->clock > max_dotclk)
> +	if (mode_rate > link_max_data_rate || mode->clock > max_dotclk)
>  		return MODE_CLOCK_HIGH;
>  
>  	return MODE_OK;

You'll also want to fix intel_dp_mst_compute_config(). Actually that one
should check against the remaining link bw, but we don't track that
sufficiently well. Well, actually the topology manager does track it,
but just not atomically so it's not good enough in its current form.

But as a first step just checking against the total link bw would be an
improvement.

> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c2f3863..313419d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1471,6 +1471,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  bool __intel_dp_read_desc(struct intel_dp *intel_dp,
>  			  struct intel_dp_desc *desc);
>  bool intel_dp_read_desc(struct intel_dp *intel_dp);
> +int intel_dp_link_required(int pixel_clock, int bpp);
> +int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx