Re: [Intel-gfx] [PATCH] drm/i915 : Restore PWM_GRANULARITY after resume

2016-09-19 Thread Lee, Shawn C 

Understood. Thanks!

-Original Message-
From: Nikula, Jani 
Sent: Monday, September 19, 2016 5:43 PM
To: Lee, Shawn C ; intel-gfx@lists.freedesktop.org
Cc: Lee, Shawn C 
Subject: Re: [PATCH] drm/i915 : Restore PWM_GRANULARITY after resume

On Mon, 19 Sep 2016, "Lee, Shawn C"  wrote:
> From: "Lee, Shawn C" 
>
> SPT_PWM_GRANULARITY (SOUTH_CHICKEN1, bit 0) controls the granularity 
> (minimum increment) of the PWM backlight control counter. PWM 
> frequency adjustment on 128 clock increments when this bit was 1. And 
> 16 clock increments when it was 0.
>
> PWM frequency multiple octuple (from 200Hz to 1.6KHz) due to 
> SPT_PWM_GRANULARITY was clear to 0 after S3. This patch save
> SOUTH_CHICKEN1 register value before suspend. And will restore it 
> after i915 resume.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97486

Shawn, I already updated the patch. Perhaps I jumped the gun but I was 
expecting an update and didn't get one and wanted to move this forward.
That patch still has your authorship [1]. This one still has issues.

BR,
Jani.

[1] 
http://patchwork.freedesktop.org/patch/msgid/1474273457-31680-2-git-send-email-jani.nik...@intel.com

>
> Cc: Jani Nikula 
> Reviewed-by: Cooper Chiou 
> Reviewed-by: Wei Shun Chen 
> Reviewed-by: Gary C Wang 
> Signed-off-by: Shawn Lee 
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |1 +
>  drivers/gpu/drm/i915/intel_panel.c |   30 --
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 570a7ca..c4bc690 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -256,6 +256,7 @@ struct intel_panel {
>   u32 level;
>   u32 min;
>   u32 max;
> + bool pwm_alternate_increment;
>   bool enabled;
>   bool combination_mode;  /* gen 2/4 only */
>   bool active_low_pwm;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index c10e9b0..6f8469b 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -841,7 +841,7 @@ static void lpt_enable_backlight(struct 
> intel_connector *connector)  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2;
> + u32 pch_ctl1, pch_ctl2, mul;
>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   if (pch_ctl1 & BLM_PCH_PWM_ENABLE) { @@ -867,6 +867,22 @@ static 
> void lpt_enable_backlight(struct intel_connector *connector)
>  
>   /* This won't stick until the above enable. */
>   intel_panel_actually_set_backlight(connector, 
> panel->backlight.level);
> + 
> + if (HAS_PCH_LPT(dev_priv)) {
> + mul = I915_READ(SOUTH_CHICKEN2);
> + if (panel->backlight.pwm_alternate_increment)
> + mul |= LPT_PWM_GRANULARITY;
> + else
> + mul &= ~LPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN2, mul);
> + } else {
> + mul = I915_READ(SOUTH_CHICKEN1);
> + if (panel->backlight.pwm_alternate_increment)
> + mul |= SPT_PWM_GRANULARITY;
> + else
> + mul &= ~SPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN1, mul);
> + }
>  }
>  
>  static void pch_enable_backlight(struct intel_connector *connector) 
> @@ -1413,7 +1429,7 @@ static int lpt_setup_backlight(struct 
> intel_connector *connector, enum pipe unus  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2, val;
> + u32 pch_ctl1, pch_ctl2, val, mul;
>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY; @@ 
> -1436,6 +1452,16 @@ static int lpt_setup_backlight(struct 
> intel_connector *connector, enum pipe unus
>  
>   panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
>  
> + if (HAS_PCH_LPT(dev_priv))
> + mul = I915_READ(SOUTH_CHICKEN2) & LPT_PWM_GRANULARITY;
> + else
> + mul = I915_READ(SOUTH_CHICKEN1) & SPT_PWM_GRANULARITY;
> +
> + if ( mul )
> + panel->backlight.pwm_alternate_increment = true;
> + else
> + panel->backlight.pwm_alternate_increment = false;
> +
>   return 0;
>  }

--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915 : Restore PWM_GRANULARITY after resume

2016-09-19 Thread Jani Nikula 
On Mon, 19 Sep 2016, "Lee, Shawn C"  wrote:
> From: "Lee, Shawn C" 
>
> SPT_PWM_GRANULARITY (SOUTH_CHICKEN1, bit 0) controls the granularity
> (minimum increment) of the PWM backlight control counter. PWM frequency
> adjustment on 128 clock increments when this bit was 1. And 16 clock
> increments when it was 0.
>
> PWM frequency multiple octuple (from 200Hz to 1.6KHz) due to
> SPT_PWM_GRANULARITY was clear to 0 after S3. This patch save
> SOUTH_CHICKEN1 register value before suspend. And will restore
> it after i915 resume.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97486

Shawn, I already updated the patch. Perhaps I jumped the gun but I was
expecting an update and didn't get one and wanted to move this forward.
That patch still has your authorship [1]. This one still has issues.

BR,
Jani.

[1] 
http://patchwork.freedesktop.org/patch/msgid/1474273457-31680-2-git-send-email-jani.nik...@intel.com

>
> Cc: Jani Nikula 
> Reviewed-by: Cooper Chiou 
> Reviewed-by: Wei Shun Chen 
> Reviewed-by: Gary C Wang 
> Signed-off-by: Shawn Lee 
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |1 +
>  drivers/gpu/drm/i915/intel_panel.c |   30 --
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 570a7ca..c4bc690 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -256,6 +256,7 @@ struct intel_panel {
>   u32 level;
>   u32 min;
>   u32 max;
> + bool pwm_alternate_increment;
>   bool enabled;
>   bool combination_mode;  /* gen 2/4 only */
>   bool active_low_pwm;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index c10e9b0..6f8469b 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -841,7 +841,7 @@ static void lpt_enable_backlight(struct intel_connector 
> *connector)
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2;
> + u32 pch_ctl1, pch_ctl2, mul;
>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> @@ -867,6 +867,22 @@ static void lpt_enable_backlight(struct intel_connector 
> *connector)
>  
>   /* This won't stick until the above enable. */
>   intel_panel_actually_set_backlight(connector, panel->backlight.level);
> + 
> + if (HAS_PCH_LPT(dev_priv)) {
> + mul = I915_READ(SOUTH_CHICKEN2);
> + if (panel->backlight.pwm_alternate_increment)
> + mul |= LPT_PWM_GRANULARITY;
> + else
> + mul &= ~LPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN2, mul);
> + } else {
> + mul = I915_READ(SOUTH_CHICKEN1);
> + if (panel->backlight.pwm_alternate_increment)
> + mul |= SPT_PWM_GRANULARITY;
> + else
> + mul &= ~SPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN1, mul);
> + }
>  }
>  
>  static void pch_enable_backlight(struct intel_connector *connector)
> @@ -1413,7 +1429,7 @@ static int lpt_setup_backlight(struct intel_connector 
> *connector, enum pipe unus
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2, val;
> + u32 pch_ctl1, pch_ctl2, val, mul;
>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
> @@ -1436,6 +1452,16 @@ static int lpt_setup_backlight(struct intel_connector 
> *connector, enum pipe unus
>  
>   panel->backlight.enabled = pch_ctl1 & BLM_PCH_PWM_ENABLE;
>  
> + if (HAS_PCH_LPT(dev_priv))
> + mul = I915_READ(SOUTH_CHICKEN2) & LPT_PWM_GRANULARITY;
> + else
> + mul = I915_READ(SOUTH_CHICKEN1) & SPT_PWM_GRANULARITY;
> +
> + if ( mul )
> + panel->backlight.pwm_alternate_increment = true;
> + else
> + panel->backlight.pwm_alternate_increment = false;
> +
>   return 0;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915 : Restore PWM_GRANULARITY after resume

2016-09-09 Thread Jani Nikula 
On Thu, 08 Sep 2016, "Lee, Shawn C"  wrote:
> From: "Lee, Shawn C" 
>
> SPT_PWM_GRANULARITY (SOUTH_CHICKEN1, bit 0) controls the granularity
> (minimum increment) of the PWM backlight control counter. PWM frequency
> adjustment on 128 clock increments when this bit was 1. And 16 clock
> increments when it was 0.
>
> PWM frequency multiple octuple (from 200Hz to 1.6KHz) due to
> SPT_PWM_GRANULARITY was clear to 0 after S3. This patch save
> SOUTH_CHICKEN1 register value before suspend. And will restore
> it after i915 resume.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97486
>
> Cc: Jani Nikula 
> Reviewed-by: Cooper Chiou 
> Reviewed-by: Wei Shun Chen 
> Reviewed-by: Gary C Wang 
> Signed-off-by: Shawn Lee 
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |1 +
>  drivers/gpu/drm/i915/intel_panel.c |   21 +++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 570a7ca..4c28692 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -256,6 +256,7 @@ struct intel_panel {
>   u32 level;
>   u32 min;
>   u32 max;
> + u32 pwm_granularity;

None of the fields here store register information directly. Please
don't do so here either. Make this a bool. Call it
pwm_alternate_increment or something.

>   bool enabled;
>   bool combination_mode;  /* gen 2/4 only */
>   bool active_low_pwm;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index c10e9b0..720fd5a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -841,7 +841,17 @@ static void lpt_enable_backlight(struct intel_connector 
> *connector)
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2;
> + u32 pch_ctl1, pch_ctl2, mul;
> +
> + if (HAS_PCH_LPT(dev_priv)) {
> + mul = I915_READ(SOUTH_CHICKEN2);
> + mul &= ~LPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN2, mul | 
> panel->backlight.pwm_granularity);
> + } else {
> + mul = I915_READ(SOUTH_CHICKEN1);
> + mul &= ~SPT_PWM_GRANULARITY;
> + I915_WRITE(SOUTH_CHICKEN1, mul | 
> panel->backlight.pwm_granularity);
> + }

It's probably safer to modify the granularity after we've ensured the
backlight is disabled below.

>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> @@ -1413,7 +1423,7 @@ static int lpt_setup_backlight(struct intel_connector 
> *connector, enum pipe unus
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
> - u32 pch_ctl1, pch_ctl2, val;
> + u32 pch_ctl1, pch_ctl2, val, mul;
>  
>   pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
>   panel->backlight.active_low_pwm = pch_ctl1 & BLM_PCH_POLARITY;
> @@ -1421,6 +1431,13 @@ static int lpt_setup_backlight(struct intel_connector 
> *connector, enum pipe unus
>   pch_ctl2 = I915_READ(BLC_PWM_PCH_CTL2);
>   panel->backlight.max = pch_ctl2 >> 16;
>  
> + if (HAS_PCH_LPT(dev_priv))
> + mul = I915_READ(SOUTH_CHICKEN2) & LPT_PWM_GRANULARITY;
> + else
> + mul = I915_READ(SOUTH_CHICKEN1) & SPT_PWM_GRANULARITY;
> +
> + panel->backlight.pwm_granularity = mul;

In the function, there's a sequence of figuring out the max backlight,
please don't put this stuff in the middle of it. You can just make this
the first thing.

As follow-up, you can then use the field in spt_hz_to_pwm and
lpt_hz_to_pwm instead of re-reading the chicken bits there.

> +
>   if (!panel->backlight.max)
>   panel->backlight.max = get_backlight_max_vbt(connector);

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx