| Message ID | 1443742613-24748-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 02 Oct 2015, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@intel.com> > > backlight minimum is a valid value so you don't need to set maximum. > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.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 4d28c7b..d509904 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > > WARN_ON(panel->backlight.max == 0); > > - if (panel->backlight.level <= panel->backlight.min) { > + if (panel->backlight.level < panel->backlight.min) { > panel->backlight.level = panel->backlight.max; > if (panel->backlight.device) > panel->backlight.device->props.brightness = This is policy in action in the kernel. We have this whole if block here because some, uh, misguided userspace sets brightness to minimum before switching the backlight off, and assumes enabling backlight cranks up the brightness as well. (*) The condition used to be (level == 0), but since the minimum can now also be non-zero, we check (level <= min). If we changed this to (level < min), we'd regress the misguided userspaces, especially if min == 0. See commit 13f3fbe827d09e3182023c8c54058cbf97aa146e Author: Jeremiah Mahler <jmmahler@gmail.com> Date: Mon Jan 12 11:01:03 2015 -0800 drm/i915: fix inconsistent brightness after resume I'd really love to to just nuke the if block completely, but no regressions and all. BR, Jani. (*) Imagine an amplifier that would set the volume to max if it was powered on with the volume knob turned to zero...
On Fri, Oct 02, 2015 at 10:50:50AM +0300, Jani Nikula wrote: > On Fri, 02 Oct 2015, clinton.a.taylor@intel.com wrote: > > From: Clint Taylor <clinton.a.taylor@intel.com> > > > > backlight minimum is a valid value so you don't need to set maximum. > > > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.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 4d28c7b..d509904 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector) > > > > WARN_ON(panel->backlight.max == 0); > > > > - if (panel->backlight.level <= panel->backlight.min) { > > + if (panel->backlight.level < panel->backlight.min) { > > panel->backlight.level = panel->backlight.max; > > if (panel->backlight.device) > > panel->backlight.device->props.brightness = > > This is policy in action in the kernel. We have this whole if block here > because some, uh, misguided userspace sets brightness to minimum before > switching the backlight off, and assumes enabling backlight cranks up > the brightness as well. (*) If you mean X, no. It has to set the backlight because on some intel models the backlight is not routed through i915 and so it needs to explicitly control it. On resume, it sets the previous value. This is just a broken kernel. -Chris
On Fri, 02 Oct 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Oct 02, 2015 at 10:50:50AM +0300, Jani Nikula wrote: >> On Fri, 02 Oct 2015, clinton.a.taylor@intel.com wrote: >> > From: Clint Taylor <clinton.a.taylor@intel.com> >> > >> > backlight minimum is a valid value so you don't need to set maximum. >> > >> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.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 4d28c7b..d509904 100644 >> > --- a/drivers/gpu/drm/i915/intel_panel.c >> > +++ b/drivers/gpu/drm/i915/intel_panel.c >> > @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector) >> > >> > WARN_ON(panel->backlight.max == 0); >> > >> > - if (panel->backlight.level <= panel->backlight.min) { >> > + if (panel->backlight.level < panel->backlight.min) { >> > panel->backlight.level = panel->backlight.max; >> > if (panel->backlight.device) >> > panel->backlight.device->props.brightness = >> >> This is policy in action in the kernel. We have this whole if block here >> because some, uh, misguided userspace sets brightness to minimum before >> switching the backlight off, and assumes enabling backlight cranks up >> the brightness as well. (*) > > If you mean X, no. It has to set the backlight because on some intel > models the backlight is not routed through i915 and so it needs to > explicitly control it. On resume, it sets the previous value. This is > just a broken kernel. Okay, I guess I'm willing to give this a shot. This may result in regression reports, but let's see. However, the meaning of the whole check changes. What Clint now checks (level below minimum) shouldn't happen, something failed already. Maybe we should set the level to minimum instead, with debug logging? BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 4d28c7b..d509904 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1069,7 +1069,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector) WARN_ON(panel->backlight.max == 0); - if (panel->backlight.level <= panel->backlight.min) { + if (panel->backlight.level < panel->backlight.min) { panel->backlight.level = panel->backlight.max; if (panel->backlight.device) panel->backlight.device->props.brightness =