Re: [PATCH] drm/i915 Trouble with minimum brightness

[Jay Aurabind]

On 12/01/2014 03:04 PM, Jani Nikula wrote:
> On Fri, 28 Nov 2014, Jay Aurabind <mail@xxxxxxxxxxxx> wrote:
>> Hello all,
>>
>> I notice that some activity has been going on with the minimum value
>> of display brightness recently (e1c412e7575).
>>
>> But the minimum value thats currently chosen is not at all acceptable
>> for my eyes. My display is working perfectly without that restriction
>> on minimum intensity.  I tend to stare at the screen a lot and the
>> current minimum settings is straining my eyes.
>>
>> Even if you say that it may not be good for the devices, I still
>> insist, because I want my *display* to fail first, not my eyes.  So
>> please try providing a way for the needy users to override this
>> minimum settings. I hope adding a module parameter would be easy fix.
>>
>> Something like this: ? (I dont call myself a kernel programmer yet,
>> just scratching the surface)
>>
>>
>> Provide provision for users to disable restriction on minimum brightness
>> value introduced in:
>>
>>     commit e1c412e75754ab7b7002f3e18a2652d999c40d4b
>>     Author: Jani Nikula <jani.nikula@xxxxxxxxx>
>>     Date:   Wed Nov 5 14:46:31 2014 +0200
>>
>>         drm/i915: safeguard against too high minimum brightness
>>
>> There are systems which work reliably without restriction on minimum
>> value of display brightness. Also the arbitrary value may be too high
>> for many users as well.
> 
> Please file a new bug at [1], reference this mail, and attach
> /sys/kernel/debug/dri/0/i915_opregion.

Thank you for the response. But the file you mentioned to attach seems to be
a binary file, because I'm getting lot of junk characters. Is this normal ?

> 
> The minimum value is chosen and provided by the OEM. There's still some
> open questions about the interpretation of the value though (which lead
> to the commit you referenced), so I'm hesitant to make changes before we
> have those cleared up.

From a user's point of view, the reason many people and myself stick to linux
is the flexibility it provides and the power the user has, and when 
you introduce a new policy without an easy "roll back" to get back the 
previous "mechanism", I would like to let you know that it matters.


> In general I am not fond of adding new module parameters for tuning
> things. Every new knob is another point of failure that we need to test,
> and they are pretty much part of the ABI we can't easily drop or change.

Would it be difficult to remove a parameter, if it is marked "experimental" ?

Is there a fix this without changing ABI ? I mean can you make this policy
apply only on those hardware which seems to have a problem with the value of
minimum backlight? Since they are the minority, why should a policy for fixing them 
affect the rest of us ?


--
Thanks and Regards,
Aurabindo J

> 
> BR,
> Jani.
> 
> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
> 
> 
>>
>> Signed-off-by: Aurabindo J <mail@xxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>>  drivers/gpu/drm/i915/i915_params.c |  6 ++++++
>>  drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++----
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 16a6f6d..55d2ead 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2213,6 +2213,7 @@ struct i915_params {
>>  	int disable_power_well;
>>  	int enable_ips;
>>  	int invert_brightness;
>> +	int restrict_min_brightness;
>>  	int enable_cmd_parser;
>>  	/* leave bools at the end to not create holes */
>>  	bool enable_hangcheck;
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index c91cb20..0601c2a 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -46,6 +46,7 @@ struct i915_params i915 __read_mostly = {
>>  	.prefault_disable = 0,
>>  	.reset = true,
>>  	.invert_brightness = 0,
>> +	.restrict_min_brightness = 1,
>>  	.disable_display = 0,
>>  	.enable_cmd_parser = 1,
>>  	.disable_vtd_wa = 0,
>> @@ -155,6 +156,11 @@ MODULE_PARM_DESC(invert_brightness,
>>  	"to dri-devel@xxxxxxxxxxxxxxxxxxxxx, if your machine needs it. "
>>  	"It will then be included in an upcoming module version.");
>>
>> +module_param_named(restrict_min_brightness, i915.restrict_min_brightness, int, 0600);
>> +MODULE_PARM_DESC(restrict_min_brightness,
>> +	"Restrict minimum brightness "
>> +	"(-1 disable restriction, 0 value from VBT, 1 arbitrary value )");
>> +
>>  module_param_named(disable_display, i915.disable_display, bool, 0600);
>>  MODULE_PARM_DESC(disable_display, "Disable display (default: false)");
>>
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index 41b3be2..def9f4e 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -1109,10 +1109,16 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>>  	 * 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);
>> -	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);
>> +	if (i915.restrict_min_brightness < 0)
>> +		min = 0;
>> +	else if (i915.restrict_min_brightness == 0)
>> +		min = dev_priv->vbt.backlight.min_brightness;
>> +	else {
>> +		min = clamp_t(int, dev_priv->vbt.backlight.min_brightness, 0, 64);
>> +		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] */
>> -- 
>> 2.1.3
>>
>>
>>
>>
>>
>

Attachment: signature.asc
Description: OpenPGP digital signature