| Message ID | 874mgvrtzh.fsf@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
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);