Re: [PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use

From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use
Date: Sat, 23 Mar 2013 13:27:18 +0100	[thread overview]
Message-ID: <20130323122718.GY9021@phenom.ffwll.local> (raw)
In-Reply-To: <CA+gsUGSY9RZ5pjvA9kBEQt_TmA6noDJESs0wn1NdNpVTe5QEfA@mail.gmail.com>

On Fri, Mar 22, 2013 at 06:14:25PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/8 Ben Widawsky <ben@bwidawsk.net>:
> > On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote:
> >> Our static analysis tool noticed that 'reg' could be used uninitialized if
> >> we are trying to get a PLL to drive VGA and SPLL is already in use
> >> (plls->spll_refcoung != 0).
> 
> This was also reported by a human a few weeks ago and went to my TODO
> list. Thanks for clearing my TODO list :)
> 
> >>
> >> In the (error) case above, let's return false to the caller and emit an
> >> error.
> >>
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index a1505e3..0eab200 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> >>                       plls->spll_refcount++;
> >>                       reg = SPLL_CTL;
> >>                       intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
> >> +             } else {
> >> +                     DRM_ERROR("SPLL already in use\n");
> >> +                     return false;
> >>               }
> >>
> >>               WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
> >
> > I know very little about this code, but the warning still seems valuable
> > to me.
> 
> The WARN will still be hit in case no one is using SPLL but it's
> enabled. It's a different problem.
> 
> The code added by Damien is correct, but it should never happen with
> the current code because only the analog encoder tries to use SPLL and
> there's only 1 analog encoder. But we can always introduce bugs to
> trigger these errors :)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch