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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 31, 2012 at 11:31:50AM +0100, Chris Wilson wrote:
> On Thu, 31 May 2012 12:03:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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.
> 
> Right, missed that adjusted_mode->clock was rewritten in dp_mode_fixup.
> In fact, wouldn't that make the better export from intel_dp?
>    target_clock = intel_dp_link_clock(adjusted_mode);
> with a judicious adjusted_mode->private_flags |= INTEL_DP_LINK_BW_2_7/1_8?

I've considered this but then freaked out thinking about hunting down all
the places we use adjusted_mode->clock and checking whether I need to
change something ... So I've opted for the more minimal "mode->clock"
grep.

Call me a wimp, but before I try to slaugther that dragon I'd like to have
a more clear understanding of how this actually works and what it best
should look like.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48