Re: [PATCH] drm/i915: Prevent unbounded wm results in g4x_compute_wm()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 07, 2017 at 12:56:23PM +0000, Chris Wilson wrote:
> Smatch warns of
> 
> drivers/gpu/drm/i915/intel_pm.c:1161 g4x_compute_wm() warn: signedness bug returning '(-33554430)'
> 
> which is a result of it believing that wm may be INT_MAX following
> g4x_tlb_miss_wa(). Just declaring g4x_tlb_miss_wa() as returning an
> unsigned integer is not sufficient, we need to tell smatch that wm itself
> is unsigned for it to not worry. So mark up the locals we expect to be
> non-negative, and so silence smatch.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6e1358d4e764..403097606322 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -928,7 +928,7 @@ static void pineview_update_wm(struct intel_crtc *unused_crtc)
>   *  and the size of 8 whole lines. This adjustment is always performed
>   *  in the actual pixel depth regardless of whether FBC is enabled or not."
>   */
> -static int g4x_tlb_miss_wa(int fifo_size, int width, int cpp)
> +static unsigned int g4x_tlb_miss_wa(int fifo_size, int width, int cpp)
>  {
>  	int tlb_miss = fifo_size * 64 - width * cpp * 8;

Yep. This should be the only part where we depend on signed math,
so the patch lgtm.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'd like to see the vlv code changed in the same way though,
for consistency.

>  
> @@ -1105,8 +1105,8 @@ static uint16_t g4x_compute_wm(const struct intel_crtc_state *crtc_state,
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	const struct drm_display_mode *adjusted_mode =
>  		&crtc_state->base.adjusted_mode;
> -	int clock, htotal, cpp, width, wm;
> -	int latency = dev_priv->wm.pri_latency[level] * 10;
> +	unsigned int latency = dev_priv->wm.pri_latency[level] * 10;
> +	unsigned int clock, htotal, cpp, width, wm;
>  
>  	if (latency == 0)
>  		return USHRT_MAX;
> @@ -1145,7 +1145,7 @@ static uint16_t g4x_compute_wm(const struct intel_crtc_state *crtc_state,
>  		   level == G4X_WM_LEVEL_NORMAL) {
>  		wm = intel_wm_method1(clock, cpp, latency);
>  	} else {
> -		int small, large;
> +		unsigned int small, large;
>  
>  		small = intel_wm_method1(clock, cpp, latency);
>  		large = intel_wm_method2(clock, htotal, width, cpp, latency);
> @@ -1158,7 +1158,7 @@ static uint16_t g4x_compute_wm(const struct intel_crtc_state *crtc_state,
>  
>  	wm = DIV_ROUND_UP(wm, 64) + 2;
>  
> -	return min_t(int, wm, USHRT_MAX);
> +	return min_t(unsigned int, wm, USHRT_MAX);
>  }
>  
>  static bool g4x_raw_plane_wm_set(struct intel_crtc_state *crtc_state,
> -- 
> 2.15.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx