Re: [PATCH] drm/i915: compute the target_clock for edp directly

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 31, 2012 at 10:49:59AM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 15:34:20 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Instead of abusing into mode->clock, because we should touch that
> > one at all. First prep step to constify the mode argument to the
> > intel_dp_mode_fixup function.
> > 
> > The next patch will stop us from modifying mode->clock.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
> >  drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
> >  3 files changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9147894..a5d06ed 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >  	/* CPU eDP doesn't require FDI link, so just set DP M/N
> >  	   according to current link config */
> >  	if (is_cpu_edp) {
> > -		target_clock = mode->clock;
> >  		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> >  	} else {
> > -		/* [e]DP over FDI requires target mode clock
> > -		   instead of link clock */
> > -		if (is_dp)
> > -			target_clock = mode->clock;
> > -		else
> > -			target_clock = adjusted_mode->clock;
> > -
> >  		/* FDI is a binary signal running at ~2.7GHz, encoding
> >  		 * each output octet as 10 bits. The actual frequency
> >  		 * is stored as a divider into a 100MHz clock, and the
> > @@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(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);
> 
> Given the edp_encoder, we know that adjusted_mode contains either the
> fixed_panel clock or the original clock depending on whether a fixed
> mode was found. So the layering violation could be dropped here in
> favour of target_clock = adjusted_mode->clok.

It's not that simple. Assuming I understand this maze correctly, we're
dealing with 3 different clocks.
- The dp link clock, both the old and new code store that in
  adjusted_mode->clock at the end of intel_dp_mode_fixup.
- The dotclock of the scanned-out region, i.e. what we have in mode->clock
  before any fixup happens. No one cares about that one because we don't
  need to program that anywhere (the panel fitter doesn't need it to do
  it's job).
- The dotclock of the displayed mode, i.e. what comes out after the panel
  fitting. The old code stored that in mode->clock (simply because that
  was available I guess). The new code recomputes it (which is rather
  simple because we only use the panel fitter for laptop panels and not to
  e.g. upscale progressive content to a 1080i TV which can't display
  1080p).

If I understand things correctly, we need to program the link clock into
the pch pll, but the fdi clock actually wants the dotclock of the
displayed mode. We store the later in target_clock in ilk_crtc_mode_set.

Obviously, given the complexity and the rampant bad naming schemes for
the involved variables I'm pretty sure I'm still getting this wrong
somewhere ...

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48