[Intel-gfx] [PATCH v3] drm/i915: keep backlight_enable on until turn eDP display off

[Jani Nikula]

On Tue, 22 Jun 2021, Lee Shawn C <shawn.c.lee at 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.
>
> v2: modify the quirk name and debug messages.
>     unregister backlight.power callback for specific device.
>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou at intel.com>
>
> Signed-off-by: Lee Shawn C <shawn.c.lee at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     |  3 +-
>  drivers/gpu/drm/i915/display/intel_quirks.c | 34 +++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h             |  1 +
>  3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6cc03b9e4321..d3312b9bcc6f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5238,7 +5238,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) {
> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c
> index 98dd787b00e3..5f3cb006db01 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_no_pps_backlight_power_hook(struct drm_i915_private *i915)
> +{
> +	i915->quirks |= QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK;
> +	drm_info(&i915->drm, "Applying no pps backlight power 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 intel_dmi_no_pps_backlight(const struct dmi_system_id *id)
> +{
> +	DRM_INFO("This workaround prevented panel backlight issue on %s device\n", id->ident);

Not sure I like the verbose "This workaround ..." style messages.

Contrast with what the messages for reverse polarity would be via dmi:

"Backlight polarity reversed on %s\n"
->
"applying inverted panel brightness quirk\n"

> +	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 = intel_dmi_no_pps_backlight,
> +				.ident = "Google Lillipup",
> +				.matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"),
> +					    DMI_MATCH(DMI_BOARD_NAME, "Lindar"),
> +					    DMI_MATCH(DMI_PRODUCT_SKU, "sku524294"),
> +				},
> +			},
> +			{
> +				.callback = intel_dmi_no_pps_backlight,
> +				.ident = "Google Lillipup",
> +				.matches = {DMI_MATCH(DMI_BOARD_VENDOR, "Google"),
> +					    DMI_MATCH(DMI_BOARD_NAME, "Lindar"),
> +					    DMI_MATCH(DMI_PRODUCT_SKU, "sku524295"),
> +				},

I think you should use DMI_EXACT_MATCH instead of DMI_MATCH. The former
users strcmp while the latter uses strstr for comparison.

The .ident string should probably be different for the two cases;
otherwise the DRM_INFO() logging above does not differentiate between
the two.

Sorry for not catching all of these at v1 review. Anyway, this is just
small details, the approach is fine.


BR,
Jani.

> +			},
> +			{ }
> +		},
> +		.hook = quirk_no_pps_backlight_power_hook,
> +	},
>  };
>  
>  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..5a065be0792a 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_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8)
>  
>  struct intel_fbdev;
>  struct intel_fbc_work;

-- 
Jani Nikula, Intel Open Source Graphics Center