Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA

From: Daniel Vetter <daniel@ffwll.ch>
To: "U. Artie Eoff" <ullysses.a.eoff@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA
Date: Tue, 30 Sep 2014 10:04:37 +0200	[thread overview]
Message-ID: <20140930080437.GD24667@phenom.ffwll.local> (raw)
In-Reply-To: <1412030973-16738-2-git-send-email-ullysses.a.eoff@intel.com>

On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote:
> Improper truncated integer division in the scale() function causes
> actual_brightness != brightness. This (partial) work-around should be
> sufficient for a majority of use-cases, but it is by no means a complete
> solution.
> 
> TODO: Determine how best to scale "user" values to "hw" values, and
> vice-versa, when the ranges are of different sizes. That would be a
> buggy scenario even with this work-around.
> 
> The issue was introduced in the following (v3.17-rc1) commit:
> 
>     6dda730 drm/i915: respect the VBT minimum backlight brightness
> 
> v2: (thanks to Chris Wilson) clarify commit message, use rounded division
> macro
> 
> v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani)
>     -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien)
>     -v1 and v2 originally authored by Joe Konno.
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>

Is there some bug report, internal jira, mailing list reference or similar
about this?

Note that at least for OTC jira tasks we now want them to be added to
commit message with e.g.

OTC-Jria: VIZ-4932

And I guess I should merge patch 2 before patch 1, right?

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_panel.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index f17ada3..f7da913 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -398,6 +398,9 @@ intel_panel_detect(struct drm_device *dev)
>  	}
>  }
>  
> +#define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> +
>  /**
>   * scale - scale values from one range to another
>   *
> @@ -419,9 +422,8 @@ static uint32_t scale(uint32_t source_val,
>  	source_val = clamp(source_val, source_min, source_max);
>  
>  	/* avoid overflows */
> -	target_val = (uint64_t)(source_val - source_min) *
> -		(target_max - target_min);
> -	do_div(target_val, source_max - source_min);
> +	target_val = DIV_ROUND_CLOSEST_ULL((uint64_t)(source_val - source_min) *
> +			(target_max - target_min), source_max - source_min);
>  	target_val += target_min;
>  
>  	return target_val;
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch