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

From: Paulo Zanoni <przanoni@gmail.com>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: 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: Fri, 22 Mar 2013 18:14:25 -0300	[thread overview]
Message-ID: <CA+gsUGSY9RZ5pjvA9kBEQt_TmA6noDJESs0wn1NdNpVTe5QEfA@mail.gmail.com> (raw)
In-Reply-To: <20130308210656.GD31789@bwidawsk.net>

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>

>
> How about instead of else, move the reg = SPLL_CTL above the 'if'
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Paulo Zanoni