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

[Lee, Shawn C]

On Thu, 24 June 2021, Jani Nikula <jani.nikula at linux.intel.com> wrote:
>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"
>

The hook function will print messages to indicate driver had this quirk and it should be enough.
I will remove this debug output DRM_INFO("This ....").

>> +	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.
>

OK! Let me confirm with ODM team. We can have differnt .ident string (+ sku1, sku2, etc...).

Best regards,
Shawn

>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