diff mbox series

drm/i915: keep backlight_enable on until turn eDP display off

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

Commit Message

Lee Shawn C June 22, 2021, 1:31 p.m. UTC
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(-)

Comments

Jani Nikula June 22, 2021, 2:17 p.m. UTC | #1
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;
Lee Shawn C June 22, 2021, 3:05 p.m. UTC | #2
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 mbox series

Patch

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;