Re: [PATCH 3/8] drm/i915: track dp target_clock in pipe_config

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 28 Mar 2013 10:41:58 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We need it in the fdi m_n computation, which nicely kills almost
> all ugly special cases in there.
> 
> It looks like we also need this to handle 12bpc hdmi correctly.
> 
> Eventually it might be better to switch things around and put the
> target clock into adjusted_mode->clock and create a new pipe_config
> parameter for the port link clock.
> 
> v2: Add a massive comment in the code to explain this mess.
> 
> v3: s/dp_target_clock/pixel_target_clock in anticipation of the hdmi
> use-case.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++----------------------
>  drivers/gpu/drm/i915/intel_dp.c      |  1 +
>  drivers/gpu/drm/i915/intel_drv.h     |  7 ++++++-
>  3 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dfa8919..464eb90 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5356,25 +5356,9 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_display_mode *adjusted_mode =
>  		&intel_crtc->config.adjusted_mode;
> -	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> -	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
>  	struct intel_link_m_n m_n = {0};
>  	int target_clock, lane, link_bw;
> -	bool is_dp = false, is_cpu_edp = false;
> -
> -	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> -		switch (intel_encoder->type) {
> -		case INTEL_OUTPUT_DISPLAYPORT:
> -			is_dp = true;
> -			break;
> -		case INTEL_OUTPUT_EDP:
> -			is_dp = true;
> -			if (!intel_encoder_is_pch_edp(&intel_encoder->base))
> -				is_cpu_edp = true;
> -			edp_encoder = intel_encoder;
> -			break;
> -		}
> -	}
> +	uint32_t bps;
>  
>  	/* FDI is a binary signal running at ~2.7GHz, encoding
>  	 * each output octet as 10 bits. The actual frequency
> @@ -5385,11 +5369,8 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  	 */
>  	link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
>  
> -	/* [e]DP over FDI requires target mode clock instead of link clock. */
> -	if (edp_encoder)
> -		target_clock = intel_edp_target_clock(edp_encoder, mode);
> -	else if (is_dp)
> -		target_clock = mode->clock;
> +	if (intel_crtc->config.pixel_target_clock)
> +		target_clock = intel_crtc->config.pixel_target_clock;
>  	else
>  		target_clock = adjusted_mode->clock;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ef680d5..b1bf00b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -753,6 +753,7 @@ found:
>  	intel_dp->lane_count = lane_count;
>  	adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>  	pipe_config->pipe_bpp = bpp;
> +	pipe_config->pixel_target_clock = target_clock;
>  
>  	DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
>  		      intel_dp->link_bw, intel_dp->lane_count,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 41fabcb..2a86a12 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -198,7 +198,12 @@ struct intel_crtc_config {
>  	bool dither;
>  	int pipe_bpp;
>  	struct intel_link_m_n dp_m_n;
> -
> +	/**
> +	 * This is currently used by DP and HDMI encoders since those can have a
> +	 * target pixel clock != the port link clock (which is currently stored
> +	 * in adjusted_mode->clock).
> +	 */
> +	int pixel_target_clock;
>  	/* Used by SDVO (and if we ever fix it, HDMI). */
>  	unsigned pixel_multiplier;
>  };

Since you already explained there are other callers of
edp_target_clock():
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center