Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Mar 2022 23:22:01 +0200
From:   Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To:     trix@redhat.com
Cc:     jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
        rodrigo.vivi@intel.com, tvrtko.ursulin@linux.intel.com,
        airlied@linux.ie, daniel@ffwll.ch, nathan@kernel.org,
        ndesaulniers@google.com, matthew.d.roper@intel.com,
        lucas.demarchi@intel.com, airlied@redhat.com, imre.deak@intel.com,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH] drm/i915: rework the error handling in *_dpll_params
Message-ID: <YiKC+Zb2pHxQOLpO@intel.com>
References: <20220304210355.608898-1-trix@redhat.com>
 <YiKAmhXEye0fpAyF@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YiKAmhXEye0fpAyF@intel.com>
Precedence: bulk

On Fri, Mar 04, 2022 at 11:11:54PM +0200, Ville Syrj?l? wrote:
> On Fri, Mar 04, 2022 at 01:03:55PM -0800, trix@redhat.com wrote:
> > From: Tom Rix <trix@redhat.com>
> > 
> > Clang static analysis reports this issue
> > intel_dpll.c:472:31: warning: The left operand of '-'
> >   is a garbage value [core.UndefinedBinaryOperatorResult]
> >   this_err = abs(clock.dot - target);
> >                  ~~~~~~~~~ ^
> > 
> > In a loop clock.dot is set on successful call to
> > i9xx_calc_dpll_params().  If the call fails, the later
> > *is_valid() will use the previous loop's clock.dot.
> 
> I don't think this can happen. intel_pll_is_valid() validates
> all the dividers first and bails out if they are junk.

Hmm. That said, there is actually a case to be made for fully
initializing the struct, and even removing the WARN. If the
BIOS (or whatever was doing stuff before i915 takes over)
has misprogrammed the DPLL then we could potentially have
garbage dividers on our hands, and during readout we'd just
blindly call *_calc_dpll_params() on them.

So I'm thinking something along the lines of
 clock->vco = <divisor> ? DIV_ROUND_CLOSEST(...) : 0;
 clock->dot = <divisor> ? DIV_ROUND_CLOSEST(...) : 0;
might be what we should do here.

To make it a bit less ugly a small helper function might
be in order. intel_pll_div() perhaps?

> 
> > 
> > The *_dpll_params functions return an arithmetic statement
> > with the clock.dot as the variable.  Change the error handler
> > to set clock.dot to 0 and jump to the return statement.
> > 
> > Fixes: dccbea3b0704 ("drm/i915: calculate the port clock rate along with other PLL params")
> > Signed-off-by: Tom Rix <trix@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll.c | 32 ++++++++++++++---------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> > index 0ae37fdbf2a5b..ba7cada704288 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> > @@ -309,11 +309,13 @@ int pnv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m2 + 2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot;
> >  }
> >  
> > @@ -326,11 +328,13 @@ int i9xx_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = i9xx_dpll_compute_m(clock);
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot;
> >  }
> >  
> > @@ -338,11 +342,13 @@ int vlv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot / 5;
> >  }
> >  
> > @@ -350,12 +356,14 @@ int chv_calc_dpll_params(int refclk, struct dpll *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> > -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return 0;
> > +	if (WARN_ON(clock->n == 0 || clock->p == 0)) {
> > +		clock->dot = 0;
> > +		goto end;
> > +	}
> >  	clock->vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(refclk, clock->m),
> >  					   clock->n << 22);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > -
> > +end:
> >  	return clock->dot / 5;
> >  }
> >  
> > -- 
> > 2.26.3
> 
> -- 
> Ville Syrj?l?
> Intel

-- 
Ville Syrj?l?
Intel