Re: [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 03, 2013 at 01:39:27PM -0300, Paulo Zanoni wrote:
> 2013/6/3 Paulo Zanoni <przanoni@gmail.com>:
> > 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> >> We currently mutliply the link_bw of the fdi link with the pixel
> >> multiplier, which is wrong: The FDI link doesn't suddenly grow more
> >> bandwidth. In reality the pixel mutliplication only happens in the PCH,
> >> before the pixels are fed into the port.
> >>
> >> But since we our code treats the uses the target clock after pixels
> >> are doubled (tripled, ...) already, we need to correct this.
> >>
> >> Semantically it's clearer to divide the target clock to get the fdi
> >> dotclock instead of multiplying the bw, so do that instead.
> >>
> >> Note that the target clock is already multiplied by the same factor,
> >> so the division will never loose accuracy for the M/N computation.
> >>
> >> The lane computation otoh used the wrong value, we also need to feed
> >> the fdi dotclock to that.
> >>
> >> Split out on a request from Paulo Zanoni.
> >>
> >> v2: Also fix the lane computation.
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Do we ever test the pixel_multiplier case? Does it work at all?
> >
> > Why is intel_ddi_get_config the only get_config function that has
> > "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody
> > that don't need other values we would be able to remove many of those
> > "if (pixel_multiplier > 1)" checks.
> >
> > Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo
> > Zanoni <paulo.r.zanoni@intel.com>.
> 
> Actually I noticed this patch is different from the one I reviewed on Friday.
> 
> Last Friday's patch:
> 
> 1) ironlake_get_lanes_required(target_clock, etc)
> 2) fdi_dotclock = target_clock
> 3) if (pixel_multiplier > 1) etc
> 4) intel_link_compute_m_n(fdi_dotclock)
> 
> Today's patch:
> 1) fdi_dotclock = target_clock
> 2) if (pixel_multiplier > 1) etc
> 3) ironlake_get_lanes_required(fdi_dotclock)
> 4) intel_link_compute_m_n(fdi_dotclock)
> 
> The difference is the argument used in ironlake_get_lanes_required. Is
> this intentional?

Yep, see the commit message:

"v2: Also fix the lane computation."

I might need to be a notch more verbose here ...

> 
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
> >>  1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index a29295e..761254d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >>  {
> >>         struct drm_device *dev = intel_crtc->base.dev;
> >>         struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> >> -       int target_clock, lane, link_bw;
> >> +       int target_clock, lane, link_bw, fdi_dotclock;
> >>         bool setup_ok, needs_recompute = false;
> >>
> >>  retry:
> >> @@ -4010,14 +4010,16 @@ retry:
> >>         else
> >>                 target_clock = adjusted_mode->clock;
> >>
> >> -       lane = ironlake_get_lanes_required(target_clock, link_bw,
> >> +       fdi_dotclock = target_clock;
> >> +       if (pipe_config->pixel_multiplier > 1)
> >> +               fdi_dotclock /= pipe_config->pixel_multiplier;
> >> +
> >> +       lane = ironlake_get_lanes_required(fdi_dotclock, link_bw,
> >>                                            pipe_config->pipe_bpp);
> >>
> >>         pipe_config->fdi_lanes = lane;
> >>
> >> -       if (pipe_config->pixel_multiplier > 1)
> >> -               link_bw *= pipe_config->pixel_multiplier;
> >> -       intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
> >> +       intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> >>                                link_bw, &pipe_config->fdi_m_n);
> >>
> >>         setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> >> --
> >> 1.7.11.7
> >>
> >
> >
> >
> > --
> > Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni

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