Re: [PATCH] drm/i915: Enable 5.4Ghz (HBR2) link rate for Displayport 1.2-capable devices

[Daniel Vetter <daniel@ffwll.ch>]

Looks good imo, just a bit patch polish. And maybe wait for Ville's
confirmation about the stuff we're discussing in private.
-Daniel

On Fri, Jan 17, 2014 at 09:58:03AM -0700, Todd Previte wrote:
>    For HSW+ platforms, enable the 5.4Ghz (HBR2) link rate for devices that support it. The
>    sink device must report that is supports Displayport 1.2 and the HBR2 bit rate in the
>    DPCD in order to use HBR2.
> 
>    V2:
>      - Added training pattern 3 flag to intel_dp struct
>      - Added check for appropriate hardware to 5.4Ghz link rate configuration
>      - Added a check for TPS3 supprt in the DPCD read
>      - Adjusted channel equalization to use TPS3 when appropriate
>      - Cleaned up whitespace

Commit message shouldn't be indented (git will do that when displaying a
patch automatically) and should be limited to about 70 chars (for to have
a bit of space in 80 char mails when replying). Also a white line before
the sob-section.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 30 ++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7df5085..fd87c37 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -102,7 +102,11 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp)
>  	case DP_LINK_BW_2_7:
>  		break;
>  	case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */
> -		max_link_bw = DP_LINK_BW_2_7;
> +		if ((IS_HSW(dev) || (INTEL_INFO(dev)->gen >= 8)) &&
> +		   (intel_dp->dpcd[DP_DPCD_REV] >= 0x12))

Checkpatch is still a bit unhappy about the alignment of the 2-line
condition here, and I concur - imo doing this as suggest helps readiblity
quite a bit.

> +			max_link_bw = DP_LINK_BW_5_4;
> +		else
> +			max_link_bw = DP_LINK_BW_2_7;
>  		break;
>  	default:
>  		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
> @@ -805,9 +809,10 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	int lane_count, clock;
>  	int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> -	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
> +    /* Conveniently, the link BW constants become indices with a shift...*/
> +	int max_clock = intel_dp_max_link_bw(intel_dp) >> 3;
>  	int bpp, mode_rate;
> -	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
> +	static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 };
>  	int link_avail, link_clock;
>  
>  	if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
> @@ -2621,10 +2626,15 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  	bool channel_eq = false;
>  	int tries, cr_tries;
>  	uint32_t DP = intel_dp->DP;
> +	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
> +
> +    /* Training Pattern 3 for HBR2 ot 1.2 devices that support it*/

Indentation of the comment.

> +	if (intel_dp->link_bw == DP_LINK_BW_5_4 || intel_dp->use_tps3)
> +		training_pattern = DP_TRAINING_PATTERN_3;
>  
>  	/* channel equalization */
>  	if (!intel_dp_set_link_train(intel_dp, &DP,
> -				     DP_TRAINING_PATTERN_2 |
> +				     training_pattern |
>  				     DP_LINK_SCRAMBLING_DISABLE)) {
>  		DRM_ERROR("failed to start channel equalization\n");
>  		return;
> @@ -2652,7 +2662,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  		if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>  			intel_dp_start_link_train(intel_dp);
>  			intel_dp_set_link_train(intel_dp, &DP,
> -						DP_TRAINING_PATTERN_2 |
> +						training_pattern |
>  						DP_LINK_SCRAMBLING_DISABLE);
>  			cr_tries++;
>  			continue;
> @@ -2668,7 +2678,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  			intel_dp_link_down(intel_dp);
>  			intel_dp_start_link_train(intel_dp);
>  			intel_dp_set_link_train(intel_dp, &DP,
> -						DP_TRAINING_PATTERN_2 |
> +						training_pattern |
>  						DP_LINK_SCRAMBLING_DISABLE);
>  			tries = 0;
>  			cr_tries++;
> @@ -2810,6 +2820,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  		}
>  	}
>  
> +	/* Training Pattern 3 support */
> +	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x12 &&
> +		intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED) {

Another 2-line condition which should be aligned.

> +		intel_dp->use_tps3 = true;
> +		DRM_DEBUG_KMS("Displayport TPS3 supported");
> +	} else
> +		intel_dp->use_tps3 = false;
> +
>  	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>  	      DP_DWN_STRM_PORT_PRESENT))
>  		return true; /* native DP sink */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9841f78..e934d8d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -488,6 +488,7 @@ struct intel_dp {
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
>  	bool psr_setup_done;
> +	bool use_tps3;
>  	struct intel_connector *attached_connector;
>  };
>  
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch