[Intel-gfx] [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Paulo Zanoni]

2014/1/8 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
>> state doesn't affect the result of reading the current HW state.
>>
>> This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
>> HSW and then suspend/resume.
>>
>> Cc: stable at vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
>> Tested-by: Qingshuai Tian <qingshuai.tian at intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 4ec1665..0def5ef 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>>       enum pipe pipe;
>>       struct intel_crtc *intel_crtc;
>>
>> +     dev_priv->ddi_plls.spll_refcount = 0;
>> +     dev_priv->ddi_plls.wrpll1_refcount = 0;
>> +     dev_priv->ddi_plls.wrpll2_refcount = 0;
>
> One idea I have for the longer-term is to unify the ddi pll
> refcounting/readout stuff with the logic I've created for shared pch plls.
> The pch pll sharing checks and refcount logic is now really solid and
> completely paranoid with self-checks, and it took about 10 iterations to
> get there in a mostly bug-free manner. It looks a bit like ddi pll sharing
> is on track to duplicate that, so merging them would be benificial. It
> might also help the state pre-computation stuff we still need to do for
> plls.

I'm aware that's your plan, but I'm not really sure if the advantages
beat the disadvantages. Also, I remember you said you were going to
implement that, so I was waiting.

I had just written a huge list of reasons explaining why I think we
shouldn't do what you suggested, but I erased it because I know I'm
going to get flamed: merging code like this is always better in
theory.

I also think we should consider doing the other way around: making
IBX/CPT/PPT code follow the HSW implementation model, because the HSW
implementation IMHO looks much simpler and easier to understand.

>
> Anyway, patch merged to -fixes (but I'll probably only get in after 3.14
> is out).
> -Daniel
>
>> +
>>       for_each_pipe(pipe) {
>>               intel_crtc =
>>                       to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>
>> -             if (!intel_crtc->active)
>> +             if (!intel_crtc->active) {
>> +                     intel_crtc->ddi_pll_sel = PORT_CLK_SEL_NONE;
>>                       continue;
>> +             }
>>
>>               intel_crtc->ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
>>                                                                pipe);
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni