[Intel-gfx] [PATCH 2/8] drm/i915: replace ad-hoc dual-link lvds checks

[Paulo Zanoni]

Hi

2012/11/16 Daniel Vetter <daniel.vetter at ffwll.ch>:
> On Fri, Nov 16, 2012 at 5:37 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 1ad6d34..0973391 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -690,8 +690,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>>         intel_clock_t clock;
>>>         int err = target;
>>>
>>> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
>>> -           (I915_READ(LVDS)) != 0) {
>>> +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>>
>> This chunk doesn't seem to do exactly what the commit message says. I
>> tried to git-blame the lines and got a little confused. Maybe this
>> chunk deserves its own commit with an explanation. Maybe what you
>> really want to do is to revert commit
>> 832cc28d5bc676331e6376d940ae45d5937aa688 instead of removing the line?
>> If you really want to remove the line, you may also update the comment
>> immediately below?
>
> Afaict the LVDS register check is only to make sure that we don't read
> garbage. Iow the code doesn't handle the more robust dual-link mode
> checking introduced in b03543857fd75876b96e10d4320b77
>
> If we switch over to that method to check for dual-link, we also don't
> need to do the (rather bogus imo) register check and hence can just
> drop it.
>
> I've dug around in the commit history, and the last patch to touch
> this code predates b03543857fd75876b, hence why I think we should just
> drop the register check and use the new is-dual-link function,
> assuming that this has simple been overlooked in the conversion.

Well, take a look at patch 832cc28d5bc676331e6376d940ae45d5937aa688
and then read the 4-line comment that's inside the "if" statement. If
we're going to completely remove the I915_READ line, shouldn't we also
update the comment ("if the panel is on") ?

>
> Does that make sense?
> -Daniel
>
>
>>
>> The chunks below look correct.
>>
>>>                 /*
>>>                  * For LVDS, if the panel is on, just rely on its current
>>>                  * settings for dual-channel.  We haven't figured out how to
>>> @@ -766,8 +765,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>>>                         lvds_reg = PCH_LVDS;
>>>                 else
>>>                         lvds_reg = LVDS;
>>> -               if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
>>> -                   LVDS_CLKB_POWER_UP)
>>> +               if (is_dual_link_lvds(dev_priv, lvds_reg))
>>>                         clock.p2 = limit->p2.p2_fast;
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni