| Message ID | 20181024155208.18987-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight | expand |
Quoting Ville Syrjala (2018-10-24 16:52:08) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Use intel_panel_actually_set_backlight() instead of a direct > call to pwm_config() in pwm_disable_backlight(). > > The main benefit is consistent debug logging when we turn off the > backlight. Currently we see nothing in dmesg which made me wonder > whether the backlight was even getting turned off properly. > > The second benefit is consistency; This is what we do for all > the other backlight implementations. It will also have the effect of calling intel_panel_compute_brightness(0) which one presumes is desired? Just worrying if the inverted brightness quirk is ever used with pwm. -Chris
On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-10-24 16:52:08) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Use intel_panel_actually_set_backlight() instead of a direct > > call to pwm_config() in pwm_disable_backlight(). > > > > The main benefit is consistent debug logging when we turn off the > > backlight. Currently we see nothing in dmesg which made me wonder > > whether the backlight was even getting turned off properly. > > > > The second benefit is consistency; This is what we do for all > > the other backlight implementations. > > It will also have the effect of calling > intel_panel_compute_brightness(0) which one presumes is desired? We do it for everything else so it must be good? > > Just worrying if the inverted brightness quirk is ever used with pwm. If we have to invert for normal operation I don't know why we wouldn't want to invert when shutting down the backlight.
Quoting Ville Syrjälä (2018-10-24 17:33:17) > On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjala (2018-10-24 16:52:08) > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Use intel_panel_actually_set_backlight() instead of a direct > > > call to pwm_config() in pwm_disable_backlight(). > > > > > > The main benefit is consistent debug logging when we turn off the > > > backlight. Currently we see nothing in dmesg which made me wonder > > > whether the backlight was even getting turned off properly. > > > > > > The second benefit is consistency; This is what we do for all > > > the other backlight implementations. > > > > It will also have the effect of calling > > intel_panel_compute_brightness(0) which one presumes is desired? > > We do it for everything else so it must be good? > > > > > Just worrying if the inverted brightness quirk is ever used with pwm. > > If we have to invert for normal operation I don't know why > we wouldn't want to invert when shutting down the backlight. Neither do I, just seems weird to set pwm to full to turn it off. Whatever, the disparity is silly, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, Oct 24, 2018 at 05:35:51PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-10-24 17:33:17) > > On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote: > > > Quoting Ville Syrjala (2018-10-24 16:52:08) > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Use intel_panel_actually_set_backlight() instead of a direct > > > > call to pwm_config() in pwm_disable_backlight(). > > > > > > > > The main benefit is consistent debug logging when we turn off the > > > > backlight. Currently we see nothing in dmesg which made me wonder > > > > whether the backlight was even getting turned off properly. > > > > > > > > The second benefit is consistency; This is what we do for all > > > > the other backlight implementations. > > > > > > It will also have the effect of calling > > > intel_panel_compute_brightness(0) which one presumes is desired? > > > > We do it for everything else so it must be good? > > > > > > > > Just worrying if the inverted brightness quirk is ever used with pwm. > > > > If we have to invert for normal operation I don't know why > > we wouldn't want to invert when shutting down the backlight. > > Neither do I, just seems weird to set pwm to full to turn it off. I guess it's just a matter of active low vs. active high. But yeah, not sure we should be doing this at all when going through the pwm subsystem. Looks like it already has something to handle the inverted polarity. > > Whatever, the disparity is silly, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Ta.
On Wed, 24 Oct 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Oct 24, 2018 at 05:35:51PM +0100, Chris Wilson wrote: >> Quoting Ville Syrjälä (2018-10-24 17:33:17) >> > On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote: >> > > Quoting Ville Syrjala (2018-10-24 16:52:08) >> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > >> > > > Use intel_panel_actually_set_backlight() instead of a direct >> > > > call to pwm_config() in pwm_disable_backlight(). >> > > > >> > > > The main benefit is consistent debug logging when we turn off the >> > > > backlight. Currently we see nothing in dmesg which made me wonder >> > > > whether the backlight was even getting turned off properly. >> > > > >> > > > The second benefit is consistency; This is what we do for all >> > > > the other backlight implementations. >> > > >> > > It will also have the effect of calling >> > > intel_panel_compute_brightness(0) which one presumes is desired? >> > >> > We do it for everything else so it must be good? >> > >> > > >> > > Just worrying if the inverted brightness quirk is ever used with pwm. >> > >> > If we have to invert for normal operation I don't know why >> > we wouldn't want to invert when shutting down the backlight. >> >> Neither do I, just seems weird to set pwm to full to turn it off. > > I guess it's just a matter of active low vs. active high. But yeah, not > sure we should be doing this at all when going through the pwm > subsystem. Looks like it already has something to handle the inverted > polarity. I haven't seen a valid use of invert brightness outside of PCI id 0x2a42 and haven't seen a bug report about it in years. I think otherwise we should be covered with the proper use of the polarity bit. I wouldn't oppose a patch warning about invert brightness use outside of gen4 or even more specific. On the patch at hand, Acked-by: Jani Nikula <jani.nikula@intel.com> BR, Jani. > >> >> Whatever, the disparity is silly, >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Ta.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index c860116669fa..b74f4bc7143c 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -767,7 +767,7 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta struct intel_panel *panel = &connector->panel; /* Disable the backlight */ - pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS); + intel_panel_actually_set_backlight(old_conn_state, 0); usleep_range(2000, 3000); pwm_disable(panel->backlight.pwm); }