Re: [PATCH] drm/i915: safeguard against too high minimum brightness

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 05 Nov 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 05, 2014 at 02:46:31PM +0200, Jani Nikula wrote:
>> Never trust (your interpretation of) the VBT. Regression from
>> 
>> commit 6dda730e55f412a6dfb181cae6784822ba463847
>> Author: Jani Nikula <jani.nikula@intel.com>
>> Date:   Tue Jun 24 18:27:40 2014 +0300
>> 
>>     drm/i915: respect the VBT minimum backlight brightness
>> 
>> causing div by zero if VBT minimum brightness equals maximum brightness.
>> 
>> Despite my attempts I've failed in my detective work to figure out what
>> the root cause is. This is not the real fix, but we have to do
>> something.
>> 
>> Reported-by: Mike Auty <mike.auty@gmail.com>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=86551
>> Cc: stable@vger.kernel.org (v3.17+)
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_panel.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index e18b3f49074c..b001c90312e7 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -1094,12 +1094,25 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>>  	struct drm_device *dev = connector->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_panel *panel = &connector->panel;
>> +	int min;
>>  
>>  	WARN_ON(panel->backlight.max == 0);
>>  
>> +	/*
>> +	 * XXX: If the vbt value is 255, it makes min equal to max, which leads
>> +	 * to problems. There are such machines out there. Either our
>> +	 * interpretation is wrong or the vbt has bogus data. Or both. Safeguard
>> +	 * against this by letting the minimum be at most (arbitrarily chosen)
>> +	 * 25% of the max.
>> +	 */
>> +	min = clamp_t(int, dev_priv->vbt.backlight.min_brightness, 0, 64);
>
> min_brightness is unsigned everywhere I've look, so a min_t looks
> sufficient here. Otherwise
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed to drm-intel-fixes, thanks for the review.

BR,
Jani.

>
> Also, more gin pls :(
> -Daniel
>
>> +	if (min != dev_priv->vbt.backlight.min_brightness) {
>> +		DRM_DEBUG_KMS("clamping VBT min backlight %d/255 to %d/255\n",
>> +			      dev_priv->vbt.backlight.min_brightness, min);
>> +	}
>> +
>>  	/* vbt value is a coefficient in range [0..255] */
>> -	return scale(dev_priv->vbt.backlight.min_brightness, 0, 255,
>> -		     0, panel->backlight.max);
>> +	return scale(min, 0, 255, 0, panel->backlight.max);
>>  }
>>  
>>  static int bdw_setup_backlight(struct intel_connector *connector)
>> -- 
>> 2.1.1
>> 
>> _______________________________________________
>> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx