[Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Shih-Yuan Lee (FourDollars)]

Should I improve this patch or you have rejected this patch?

Regards,
$4
On Mon, Nov 09, 2015 at 06:57:22PM +0200, Jani Nikula wrote:
> On Mon, 09 Nov 2015, Paulo Zanoni <przanoni at gmail.com> wrote:
> > 2015-11-09 8:17 GMT-02:00 Jani Nikula <jani.nikula at linux.intel.com>:
> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx