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

[Daniel Vetter]

On Wed, Jan 08, 2014 at 01:55:19PM -0200, Paulo Zanoni wrote:
> 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.

Well there's lots of stuff somewhere on my todo continually falling off
the end. So if you have some spare bandwidth just ping to make sure I
don't start at the same time. Otherwise I don't mind at all if people
steal my todo items ;-)

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

Yeah, it's fairly complex and abstracted since the older code was imo too
fragile. Imo the separation between the hw enable/disable code and the
refcounting/checking is fairly nice.

But it sounds like you disagree, so I really want to hear your reasons and
objections. Can you please dig them out again?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch