| Message ID | 1384424069-15078-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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;
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> --- 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(-)