Re: [PATCH 11.5/13] drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4518 bytes --]

On Thu, 2013-11-14 at 12:14 +0200, Jani Nikula wrote:
> The quirk was added as what I'd say was a stopgap measure in
> 
> commit e85843bec6c2ea7c10ec61238396891cc2b753a9
> Author: Kamal Mostafa <kamal@canonical.com>
> Date:   Fri Jul 19 15:02:01 2013 -0700
> 
>     drm/i915: quirk no PCH_PWM_ENABLE for Dell XPS13 backlight
> 
> without really digging into what was going on.
> 
> Also, as mentioned in the related bug [1], having the quirk regressed
> some of the machines it was supposed to fix to begin with, and there
> were patches posted to disable the quirk on such machines [2]!
> 
> The fact is, we do need the BLM_PCH_PWM_ENABLE bit set to have
> backlight. With the quirk, we've relied on BIOS to have set it, and our
> save/restore code to retain it. With the full backlight setup at enable,
> we have no place for things that rely on previous state.
> 
> With the per platform hooks, we've also made a change in the PCH
> platform enable order: setting the backlight duty cycle between CPU and
> PCH PWM enable. Some experimenting and
> 
> commit 770c12312ad617172b1a65b911d3e6564fc5aca8
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Sat Aug 11 08:56:42 2012 +0200
> 
>     drm/i915: Fix blank panel at reopening lid
> 
> indicate that we can't set the backlight before enabling CPU PWM; the
> value just won't stick. But AFAICT we should do it before enabling the
> PCH PWM.
> 
> Finally, any fallout we should fix properly, preferrably without quirks,
> and absolutely without quirks that rely on existing state. With the per
> platform hooks have much more flexibility to adjust the sequence as
> required by platforms.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=47941
> [2] http://lkml.kernel.org/r/1378229848-29113-1-git-send-email-kamal@canonical.com
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Thanks for the explanation:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 -
>  drivers/gpu/drm/i915/intel_display.c |   16 ----------------
>  drivers/gpu/drm/i915/intel_panel.c   |    4 ----
>  3 files changed, 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c243b8e..e726ab9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -717,7 +717,6 @@ enum intel_sbi_destination {
>  #define QUIRK_PIPEA_FORCE (1<<0)
>  #define QUIRK_LVDS_SSC_DISABLE (1<<1)
>  #define QUIRK_INVERT_BRIGHTNESS (1<<2)
> -#define QUIRK_NO_PCH_PWM_ENABLE (1<<3)
>  
>  struct intel_fbdev;
>  struct intel_fbc_work;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 25ef080..b9f763c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10456,17 +10456,6 @@ static void quirk_invert_brightness(struct drm_device *dev)
>  	DRM_INFO("applying inverted panel brightness quirk\n");
>  }
>  
> -/*
> - * Some machines (Dell XPS13) suffer broken backlight controls if
> - * BLM_PCH_PWM_ENABLE is set.
> - */
> -static void quirk_no_pcm_pwm_enable(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	dev_priv->quirks |= QUIRK_NO_PCH_PWM_ENABLE;
> -	DRM_INFO("applying no-PCH_PWM_ENABLE quirk\n");
> -}
> -
>  struct intel_quirk {
>  	int device;
>  	int subsystem_vendor;
> @@ -10526,11 +10515,6 @@ static struct intel_quirk intel_quirks[] = {
>  	 * seem to use inverted backlight PWM.
>  	 */
>  	{ 0x2a42, 0x1025, PCI_ANY_ID, quirk_invert_brightness },
> -
> -	/* Dell XPS13 HD Sandy Bridge */
> -	{ 0x0116, 0x1028, 0x052e, quirk_no_pcm_pwm_enable },
> -	/* Dell XPS13 HD and XPS13 FHD Ivy Bridge */
> -	{ 0x0166, 0x1028, 0x058b, quirk_no_pcm_pwm_enable },
>  };
>  
>  static void intel_init_quirks(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 0986472..da088e3 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -749,10 +749,6 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	pch_ctl2 = panel->backlight.max << 16;
>  	I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
>  
> -	/* XXX: transitional */
> -	if (dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)
> -		return;
> -
>  	pch_ctl1 = 0;
>  	if (panel->backlight.active_low_pwm)
>  		pch_ctl1 |= BLM_PCH_POLARITY;


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx