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

[Paulo Zanoni]

2013/6/3 Paulo Zanoni <przanoni at gmail.com>:
> 2013/6/1 Daniel Vetter <daniel.vetter at 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 at 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 at 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?

>
>> ---
>>  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