On Thu, 24 June 2021, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>On Tue, 22 Jun 2021, Lee Shawn C <shawn.c.lee@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx>
>>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
