Re: [PATCH] drm/i915: A better maximum brightness for users.

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

On Mon, 09 Nov 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-09 8:17 GMT-02:00 Jani Nikula <jani.nikula@linux.intel.com>:
>> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@canonical.com> wrote:
>>> The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
>>> the sysfs brightness level always starts from 0 so it is better to use
>>> 927 as the sysfs maximum brightness level and it becomes easier to map
>>> from the PWM brightness level to the sysfs brightness level.
>>
>> We've been thinking we should provide a fixed range to userspace
>> instead. Say, 0-100.
>
> While not clearly stated, this reply and the further ones sound like a
> rejection to his patch.

I wanted to understand the motivation rather than bluntly rejecting.

> I'm not really an expert on backlight, but his idea sounds simple and
> an overall improvement to the codebase. I don't think it's fair to
> block a simple one-line improvement based on the fact that we have an
> idea (that may or may not be implemented) for a possibly better
> implementation. Why not apply his patch (in case we conclude it
> doesn't have any bugs), and then go for the 0-100 idea once someone
> actually decides to implement it?

The implementation is

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a24df35e11e7..d5d86601d411 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct intel_connector *connector)
 	 * Note: Everything should work even if the backlight device max
 	 * presented to the userspace is arbitrarily chosen.
 	 */
-	props.max_brightness = panel->backlight.max;
+	props.max_brightness = 100;
 	props.brightness = scale_hw_to_user(connector,
 					    panel->backlight.level,
 					    props.max_brightness);

and "just" testing is required.

> Besides, isn't there the possibility of having a panel that has a
> 0-9999 range where the change from 0 to 9990 is negligible, and only
> 9991-9999 matters, so when we scale to 0-100 it will become basically
> a on/off switch? We all learned to never underestimate panel hardware.

That's a somewhat more valid argument. The answer to that should be that
we map the userspace range to the hardware range according to a curve
rather than a linear mapping. See Ville's reply.

BR,
Jani.



>
>>
>> BR,
>> Jani.
>>
>>
>>
>>
>>>
>>> Signed-off-by: Shih-Yuan Lee (FourDollars) <sylee@canonical.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>>> index a24df35..697fd4d 100644
>>> --- a/drivers/gpu/drm/i915/intel_panel.c
>>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>>> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>>>        * Note: Everything should work even if the backlight device max
>>>        * presented to the userspace is arbitrarily chosen.
>>>        */
>>> -     props.max_brightness = panel->backlight.max;
>>> +     props.max_brightness = panel->backlight.max - panel->backlight.min;
>>>       props.brightness = scale_hw_to_user(connector,
>>>                                           panel->backlight.level,
>>>                                           props.max_brightness);
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Paulo Zanoni

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