Re: [PATCH 3/3] drm/i915/dp: get rid of intel_dp->link_configuration

Ville Syrjälä
2013-10-04T12:44:47Z

On Fri, Oct 04, 2013 at 03:08:10PM +0300, Jani Nikula wrote:
> It's not really needed, rather just adds another place to hold
> intermediate values that could go wrong, and it's not clear that the
> training pattern set or training lane set should be written at this
> point at all.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 5 +----
> drivers/gpu/drm/i915/intel_dp.c | 34 ++++++++++++----------------------
> drivers/gpu/drm/i915/intel_drv.h | 3 ---
> 3 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a43ac3a..6d335f8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -316,9 +316,6 @@ static void intel_ddi_mode_set(struct intel_encoder *encoder)
> DRM_DEBUG_DRIVER("DP audio: write eld information\n");
> intel_write_eld(&encoder->base, adjusted_mode);
> }
> -
> - intel_dp_init_link_config(intel_dp);
> -
> } else if (type == INTEL_OUTPUT_HDMI) {
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>
> @@ -1222,7 +1219,7 @@ void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder)
>
> val = DP_TP_CTL_ENABLE | DP_TP_CTL_MODE_SST |
> DP_TP_CTL_LINK_TRAIN_PAT1 | DP_TP_CTL_SCRAMBLE_DISABLE;
> - if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> + if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
> I915_WRITE(DP_TP_CTL(port), val);
> POSTING_READ(DP_TP_CTL(port));
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 60118da..bf32eca 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -884,21 +884,6 @@ found:
> return true;
> }
>
> -void intel_dp_init_link_config(struct intel_dp *intel_dp)
> -{
> - memset(intel_dp->link_configuration, 0, DP_LINK_CONFIGURATION_SIZE);
> - intel_dp->link_configuration[0] = intel_dp->link_bw;
> - intel_dp->link_configuration[1] = intel_dp->lane_count;
> - intel_dp->link_configuration[8] = DP_SET_ANSI_8B10B;
> - /*
> - * Check for DPCD version > 1.1 and enhanced framing support
> - */
> - if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> - (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
> - intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> - }
> -}
> -
> static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> @@ -971,8 +956,6 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> intel_write_eld(&encoder->base, adjusted_mode);
> }
>
> - intel_dp_init_link_config(intel_dp);
> -
> /* Split out the IBX/CPU vs CPT settings */
>
> if (port == PORT_A && IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) {
> @@ -982,7 +965,7 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> intel_dp->DP |= DP_SYNC_VS_HIGH;
> intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
>
> - if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> + if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> intel_dp->DP |= DP_ENHANCED_FRAMING;
>
> intel_dp->DP |= crtc->pipe << 29;
> @@ -996,7 +979,7 @@ static void intel_dp_mode_set(struct intel_encoder *encoder)
> intel_dp->DP |= DP_SYNC_VS_HIGH;
> intel_dp->DP |= DP_LINK_TRAIN_OFF;
>
> - if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> + if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> intel_dp->DP |= DP_ENHANCED_FRAMING;
>
> if (crtc->pipe == 1)
> @@ -2474,14 +2457,21 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> uint8_t voltage;
> int voltage_tries, loop_tries;
> uint32_t DP = intel_dp->DP;
> + uint8_t link_config[2];
>
> if (HAS_DDI(dev))
> intel_ddi_prepare_link_retrain(encoder);
>
> /* Write the link configuration data */
> - intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET,
> - intel_dp->link_configuration,
> - DP_LINK_CONFIGURATION_SIZE);
> + link_config[0] = intel_dp->link_bw;
> + link_config[1] = intel_dp->lane_count;
> + if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> + link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;

We didn't care about the non-interoperability crap before, and I
suppose we don't care about it now. Hopefully there isn't an eDP
panel out there that requires it, and hopefully all panels also
respect the spec enough to power up w/ DP_EDP_CONFIGURATION_SET=0x0.

For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> + intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
> +
> + link_config[0] = 0;
> + link_config[1] = DP_SET_ANSI_8B10B;
> + intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
>
> DP |= DP_PORT_EN;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 922c5d7..eaf0003 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -436,13 +436,11 @@ struct intel_hdmi {
> };
>
> #define DP_MAX_DOWNSTREAM_PORTS 0x10
> -#define DP_LINK_CONFIGURATION_SIZE 9
>
> struct intel_dp {
> uint32_t output_reg;
> uint32_t aux_ch_ctl_reg;
> uint32_t DP;
> - uint8_t link_configuration[DP_LINK_CONFIGURATION_SIZE];
> bool has_audio;
> enum hdmi_force_audio force_audio;
> uint32_t color_range;
> @@ -685,7 +683,6 @@ void i915_disable_vga_mem(struct drm_device *dev);
> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> struct intel_connector *intel_connector);
> -void intel_dp_init_link_config(struct intel_dp *intel_dp);
> void intel_dp_start_link_train(struct intel_dp *intel_dp);
> void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrjälä
Intel OTC