| Message ID | 20210622133107.7422-1-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: keep backlight_enable on until turn eDP display off | expand |
On Tue, 22 Jun 2021, Lee Shawn C <shawn.c.lee@intel.com> wrote: > This workaround is specific for a particular panel on Google > chromebook project. When user space daemon enter idle state. > It request adjust brightness to 0, turn backlight_enable signal > off and keep eDP main link active. > > On general LCD, this behavior might not be a problem. > But on this panel, its tcon would expect source to execute > full eDP power off sequence after drop backlight_enable signal. > Without eDP power off sequence. Even source try to turn > backlight_enable signal on and restore proper brightness level. > This panel is not able to light on again. > > This WA ignored the request from user space daemon to disable > backlight_enable signal and keep it on always. When user space > request kernel to turn eDP display off, kernel driver still > can control backlight_enable signal properly. It would not > impact standard eDP power off sequence. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Cooper Chiou <cooper.chiou@intel.com> > > Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/i915/display/intel_panel.c | 4 ++- > drivers/gpu/drm/i915/display/intel_quirks.c | 34 +++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > 3 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c > index 7d7a60b4d2de..0212b53d932b 100644 > --- a/drivers/gpu/drm/i915/display/intel_panel.c > +++ b/drivers/gpu/drm/i915/display/intel_panel.c > @@ -1311,6 +1311,7 @@ static void intel_panel_set_backlight(const struct drm_connector_state *conn_sta > static int intel_backlight_device_update_status(struct backlight_device *bd) > { > struct intel_connector *connector = bl_get_data(bd); > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > struct intel_panel *panel = &connector->panel; > struct drm_device *dev = connector->base.dev; > > @@ -1330,7 +1331,8 @@ static int intel_backlight_device_update_status(struct backlight_device *bd) > if (panel->backlight.power) { > bool enable = bd->props.power == FB_BLANK_UNBLANK && > bd->props.brightness != 0; > - panel->backlight.power(connector, enable); > + if (enable || !(dev_priv->quirks & QUIRK_KEEP_BACKLIGHT_ENABLE_ON)) > + panel->backlight.power(connector, enable); We'll want to avoid the hook altogether in this case, instead of complicating the logic even more. Something like: --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5286,7 +5286,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, } intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode); - intel_connector->panel.backlight.power = intel_pps_backlight_power; + if (!(dev_priv->quirks & QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK)) + intel_connector->panel.backlight.power = intel_pps_backlight_power; intel_panel_setup_backlight(connector, pipe); if (fixed_mode) { --- Please adjust the quirk name and debug logs accordingly. Truth be told I'd rather nuke the hook completely. But it was a request from Chrome to have this lightweight sub-state to black out the display, without mode set, using the panel power sequencer in cases where we can't set the backlight PWM to 0. Hmm. Is the brightness controlled using PWM or DPCD? We probably shouldn't do this at all when the brightness control is done using DPCD. BR, Jani. > } > } else { > bd->props.power = FB_BLANK_POWERDOWN; > diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c > index 98dd787b00e3..ed57b083edbb 100644 > --- a/drivers/gpu/drm/i915/display/intel_quirks.c > +++ b/drivers/gpu/drm/i915/display/intel_quirks.c > @@ -53,6 +53,12 @@ static void quirk_increase_ddi_disabled_time(struct drm_i915_private *i915) > drm_info(&i915->drm, "Applying Increase DDI Disabled quirk\n"); > } > > +static void quirk_keep_backlight_enable_on(struct drm_i915_private *i915) > +{ > + i915->quirks |= QUIRK_KEEP_BACKLIGHT_ENABLE_ON; > + drm_info(&i915->drm, "applying keep backlight enable on quirk\n"); > +} > + > struct intel_quirk { > int device; > int subsystem_vendor; > @@ -72,6 +78,12 @@ static int intel_dmi_reverse_brightness(const struct dmi_system_id *id) > return 1; > } > > +static int backlight_wa_callback(const struct dmi_system_id *id) > +{ > + DRM_INFO("This is WA to ignore backlight off to prevent OLED panel issue on %s device\n", id->ident); > + return 1; > +} > + > static const struct intel_dmi_quirk intel_dmi_quirks[] = { > { > .dmi_id_list = &(const struct dmi_system_id[]) { > @@ -96,6 +108,28 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = { > }, > .hook = quirk_invert_brightness, > }, > + { > + .dmi_id_list = &(const struct dmi_system_id[]) { > + { > + .callback = backlight_wa_callback, > + .ident = "Google Lillipup", > + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), > + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), > + DMI_MATCH(DMI_PRODUCT_SKU, "sku524294"), > + }, > + }, > + { > + .callback = backlight_wa_callback, > + .ident = "Google Lillipup", > + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), > + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), > + DMI_MATCH(DMI_PRODUCT_SKU, "sku524295"), > + }, > + }, > + { } > + }, > + .hook = quirk_keep_backlight_enable_on, > + }, > }; > > static struct intel_quirk intel_quirks[] = { > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 01e11fe38642..8103919bf3b4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -467,6 +467,7 @@ struct i915_drrs { > #define QUIRK_PIN_SWIZZLED_PAGES (1<<5) > #define QUIRK_INCREASE_T12_DELAY (1<<6) > #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7) > +#define QUIRK_KEEP_BACKLIGHT_ENABLE_ON (1<<8) > > struct intel_fbdev; > struct intel_fbc_work;
On Tue, 22 Jun 2021, 10:18 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: >On Tue, 22 Jun 2021, Lee Shawn C <shawn.c.lee@intel.com> wrote: >> This workaround is specific for a particular panel on Google >> chromebook project. When user space daemon enter idle state. >> It request adjust brightness to 0, turn backlight_enable signal off >> and keep eDP main link active. >> >> On general LCD, this behavior might not be a problem. >> But on this panel, its tcon would expect source to execute full eDP >> power off sequence after drop backlight_enable signal. >> Without eDP power off sequence. Even source try to turn >> backlight_enable signal on and restore proper brightness level. >> This panel is not able to light on again. >> >> This WA ignored the request from user space daemon to disable >> backlight_enable signal and keep it on always. When user space request >> kernel to turn eDP display off, kernel driver still can control >> backlight_enable signal properly. It would not impact standard eDP >> power off sequence. >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Cooper Chiou <cooper.chiou@intel.com> >> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_panel.c | 4 ++- >> drivers/gpu/drm/i915/display/intel_quirks.c | 34 +++++++++++++++++++++ >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> 3 files changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c >> b/drivers/gpu/drm/i915/display/intel_panel.c >> index 7d7a60b4d2de..0212b53d932b 100644 >> --- a/drivers/gpu/drm/i915/display/intel_panel.c >> +++ b/drivers/gpu/drm/i915/display/intel_panel.c >> @@ -1311,6 +1311,7 @@ static void intel_panel_set_backlight(const >> struct drm_connector_state *conn_sta static int >> intel_backlight_device_update_status(struct backlight_device *bd) { >> struct intel_connector *connector = bl_get_data(bd); >> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); >> struct intel_panel *panel = &connector->panel; >> struct drm_device *dev = connector->base.dev; >> >> @@ -1330,7 +1331,8 @@ static int intel_backlight_device_update_status(struct backlight_device *bd) >> if (panel->backlight.power) { >> bool enable = bd->props.power == FB_BLANK_UNBLANK && >> bd->props.brightness != 0; >> - panel->backlight.power(connector, enable); >> + if (enable || !(dev_priv->quirks & QUIRK_KEEP_BACKLIGHT_ENABLE_ON)) >> + panel->backlight.power(connector, enable); > >We'll want to avoid the hook altogether in this case, instead of complicating the logic even more. Something like: > >--- a/drivers/gpu/drm/i915/display/intel_dp.c >+++ b/drivers/gpu/drm/i915/display/intel_dp.c >@@ -5286,7 +5286,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > } > > intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode); >- intel_connector->panel.backlight.power = intel_pps_backlight_power; >+ if (!(dev_priv->quirks & QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK)) >+ intel_connector->panel.backlight.power = intel_pps_backlight_power; > intel_panel_setup_backlight(connector, pipe); > > if (fixed_mode) { > >--- > >Please adjust the quirk name and debug logs accordingly. > Thanks! I will modify the quirk name and debug messages later. >Truth be told I'd rather nuke the hook completely. But it was a request from Chrome to have this lightweight sub-state to black out the display, without mode set, using the panel power sequencer in cases where we can't set the backlight PWM to 0. > >Hmm. Is the brightness controlled using PWM or DPCD? We probably shouldn't do this at all when the brightness control is done using DPCD. This panel refer to soc's backlight_enable signal to enable backlight. But adjust brightness via DPCD aux interface. If source turn backlight_enable signal off, panel tcon await source execute full eDP power off sequence to disable local display. And panel's backlight is not able to turn on again. Best regards, Shawn > >BR, >Jani. > > >> } >> } else { >> bd->props.power = FB_BLANK_POWERDOWN; diff --git >> a/drivers/gpu/drm/i915/display/intel_quirks.c >> b/drivers/gpu/drm/i915/display/intel_quirks.c >> index 98dd787b00e3..ed57b083edbb 100644 >> --- a/drivers/gpu/drm/i915/display/intel_quirks.c >> +++ b/drivers/gpu/drm/i915/display/intel_quirks.c >> @@ -53,6 +53,12 @@ static void quirk_increase_ddi_disabled_time(struct drm_i915_private *i915) >> drm_info(&i915->drm, "Applying Increase DDI Disabled quirk\n"); } >> >> +static void quirk_keep_backlight_enable_on(struct drm_i915_private >> +*i915) { >> + i915->quirks |= QUIRK_KEEP_BACKLIGHT_ENABLE_ON; >> + drm_info(&i915->drm, "applying keep backlight enable on quirk\n"); } >> + >> struct intel_quirk { >> int device; >> int subsystem_vendor; >> @@ -72,6 +78,12 @@ static int intel_dmi_reverse_brightness(const struct dmi_system_id *id) >> return 1; >> } >> >> +static int backlight_wa_callback(const struct dmi_system_id *id) { >> + DRM_INFO("This is WA to ignore backlight off to prevent OLED panel issue on %s device\n", id->ident); >> + return 1; >> +} >> + >> static const struct intel_dmi_quirk intel_dmi_quirks[] = { >> { >> .dmi_id_list = &(const struct dmi_system_id[]) { @@ -96,6 +108,28 >> @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = { >> }, >> .hook = quirk_invert_brightness, >> }, >> + { >> + .dmi_id_list = &(const struct dmi_system_id[]) { >> + { >> + .callback = backlight_wa_callback, >> + .ident = "Google Lillipup", >> + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), >> + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), >> + DMI_MATCH(DMI_PRODUCT_SKU, "sku524294"), >> + }, >> + }, >> + { >> + .callback = backlight_wa_callback, >> + .ident = "Google Lillipup", >> + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), >> + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), >> + DMI_MATCH(DMI_PRODUCT_SKU, "sku524295"), >> + }, >> + }, >> + { } >> + }, >> + .hook = quirk_keep_backlight_enable_on, >> + }, >> }; >> >> static struct intel_quirk intel_quirks[] = { diff --git >> a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 01e11fe38642..8103919bf3b4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -467,6 +467,7 @@ struct i915_drrs { #define >> QUIRK_PIN_SWIZZLED_PAGES (1<<5) #define QUIRK_INCREASE_T12_DELAY >> (1<<6) #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7) >> +#define QUIRK_KEEP_BACKLIGHT_ENABLE_ON (1<<8) >> >> struct intel_fbdev; >> struct intel_fbc_work; > >-- >Jani Nikula, Intel Open Source Graphics Center >
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c index 7d7a60b4d2de..0212b53d932b 100644 --- a/drivers/gpu/drm/i915/display/intel_panel.c +++ b/drivers/gpu/drm/i915/display/intel_panel.c @@ -1311,6 +1311,7 @@ static void intel_panel_set_backlight(const struct drm_connector_state *conn_sta static int intel_backlight_device_update_status(struct backlight_device *bd) { struct intel_connector *connector = bl_get_data(bd); + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_panel *panel = &connector->panel; struct drm_device *dev = connector->base.dev; @@ -1330,7 +1331,8 @@ static int intel_backlight_device_update_status(struct backlight_device *bd) if (panel->backlight.power) { bool enable = bd->props.power == FB_BLANK_UNBLANK && bd->props.brightness != 0; - panel->backlight.power(connector, enable); + if (enable || !(dev_priv->quirks & QUIRK_KEEP_BACKLIGHT_ENABLE_ON)) + panel->backlight.power(connector, enable); } } else { bd->props.power = FB_BLANK_POWERDOWN; diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c index 98dd787b00e3..ed57b083edbb 100644 --- a/drivers/gpu/drm/i915/display/intel_quirks.c +++ b/drivers/gpu/drm/i915/display/intel_quirks.c @@ -53,6 +53,12 @@ static void quirk_increase_ddi_disabled_time(struct drm_i915_private *i915) drm_info(&i915->drm, "Applying Increase DDI Disabled quirk\n"); } +static void quirk_keep_backlight_enable_on(struct drm_i915_private *i915) +{ + i915->quirks |= QUIRK_KEEP_BACKLIGHT_ENABLE_ON; + drm_info(&i915->drm, "applying keep backlight enable on quirk\n"); +} + struct intel_quirk { int device; int subsystem_vendor; @@ -72,6 +78,12 @@ static int intel_dmi_reverse_brightness(const struct dmi_system_id *id) return 1; } +static int backlight_wa_callback(const struct dmi_system_id *id) +{ + DRM_INFO("This is WA to ignore backlight off to prevent OLED panel issue on %s device\n", id->ident); + return 1; +} + static const struct intel_dmi_quirk intel_dmi_quirks[] = { { .dmi_id_list = &(const struct dmi_system_id[]) { @@ -96,6 +108,28 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = { }, .hook = quirk_invert_brightness, }, + { + .dmi_id_list = &(const struct dmi_system_id[]) { + { + .callback = backlight_wa_callback, + .ident = "Google Lillipup", + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), + DMI_MATCH(DMI_PRODUCT_SKU, "sku524294"), + }, + }, + { + .callback = backlight_wa_callback, + .ident = "Google Lillipup", + .matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"), + DMI_MATCH(DMI_BOARD_NAME, "Lindar"), + DMI_MATCH(DMI_PRODUCT_SKU, "sku524295"), + }, + }, + { } + }, + .hook = quirk_keep_backlight_enable_on, + }, }; static struct intel_quirk intel_quirks[] = { diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 01e11fe38642..8103919bf3b4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -467,6 +467,7 @@ struct i915_drrs { #define QUIRK_PIN_SWIZZLED_PAGES (1<<5) #define QUIRK_INCREASE_T12_DELAY (1<<6) #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7) +#define QUIRK_KEEP_BACKLIGHT_ENABLE_ON (1<<8) struct intel_fbdev; struct intel_fbc_work;
This workaround is specific for a particular panel on Google chromebook project. When user space daemon enter idle state. It request adjust brightness to 0, turn backlight_enable signal off and keep eDP main link active. On general LCD, this behavior might not be a problem. But on this panel, its tcon would expect source to execute full eDP power off sequence after drop backlight_enable signal. Without eDP power off sequence. Even source try to turn backlight_enable signal on and restore proper brightness level. This panel is not able to light on again. This WA ignored the request from user space daemon to disable backlight_enable signal and keep it on always. When user space request kernel to turn eDP display off, kernel driver still can control backlight_enable signal properly. It would not impact standard eDP power off sequence. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Cooper Chiou <cooper.chiou@intel.com> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> --- drivers/gpu/drm/i915/display/intel_panel.c | 4 ++- drivers/gpu/drm/i915/display/intel_quirks.c | 34 +++++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 1 + 3 files changed, 38 insertions(+), 1 deletion(-)