| Message ID | 1451556028-2431-1-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 31 Dec 2015, Vidya Srinivas <vidya.srinivas@intel.com> wrote: > Currently Backlight registers which are associated with Power Well 0 > are not being saved before gating the power well for S0ix. Hence, > upon resume from S0ix, these registers are not being restored. Due to > this, the display has resumed and since there is no backlight, nothing is > seen. Patch fixes this issue by saving/restoring BLC registers for S0ix. No, this is not the approach. The backlight disable/enable hooks should handle this by always writing the full state of the registers on enable. Doing blind register copies (and read-modify-writes) is what we've been trying to get rid of across the driver. BR, Jani. > > Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 4 ++++ > drivers/gpu/drm/i915/intel_panel.c | 18 ++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 50f83d2..752fb58 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -193,6 +193,10 @@ struct intel_panel { > uint32_t hz); > void (*power)(struct intel_connector *, bool enable); > } backlight; > + > + u32 blc_pwm_ctl; > + u32 blc_pwm_freq; > + u32 blc_pwm_duty; > }; > > struct intel_connector { > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index ae808b6..421cd3a 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -816,6 +816,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) > val &= ~UTIL_PIN_ENABLE; > I915_WRITE(UTIL_PIN_CTL, val); > } > + > + /* Saving BLC registers for PG0 gating */ > + panel->blc_pwm_ctl = > + I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller)); > + panel->blc_pwm_freq = > + I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller)); > + panel->blc_pwm_duty = > + I915_READ(BXT_BLC_PWM_DUTY(panel->backlight.controller)); > + > + > } > > static void pwm_disable_backlight(struct intel_connector *connector) > @@ -1050,6 +1060,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) > enum pipe pipe = intel_get_pipe_from_connector(connector); > u32 pwm_ctl, val; > > + /* Restore BLC registers if PG0 was gated */ > + I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller), > + panel->blc_pwm_ctl); > + I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller), > + panel->blc_pwm_freq); > + I915_WRITE(BXT_BLC_PWM_DUTY(panel->backlight.controller), > + panel->blc_pwm_duty); > + > /* To use 2nd set of backlight registers, utility pin has to be > * enabled with PWM mode. > * The field should only be changed when the utility pin is disabled
On Sat, 02 Jan 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 31 Dec 2015, Vidya Srinivas <vidya.srinivas@intel.com> wrote: >> Currently Backlight registers which are associated with Power Well 0 >> are not being saved before gating the power well for S0ix. Hence, >> upon resume from S0ix, these registers are not being restored. Due to >> this, the display has resumed and since there is no backlight, nothing is >> seen. Patch fixes this issue by saving/restoring BLC registers for S0ix. > > No, this is not the approach. > > The backlight disable/enable hooks should handle this by always writing > the full state of the registers on enable. Doing blind register copies > (and read-modify-writes) is what we've been trying to get rid of across > the driver. To be more specific, we *already* write the full state to the three registers in bxt_enable_backlight(). Your conclusions in the commit message are wrong. If you're seeing issues, it must be about the sequence in which the registers are written. Please look into that. BR, Jani. > > BR, > Jani. > > >> >> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> >> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> >> --- >> drivers/gpu/drm/i915/intel_drv.h | 4 ++++ >> drivers/gpu/drm/i915/intel_panel.c | 18 ++++++++++++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 50f83d2..752fb58 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -193,6 +193,10 @@ struct intel_panel { >> uint32_t hz); >> void (*power)(struct intel_connector *, bool enable); >> } backlight; >> + >> + u32 blc_pwm_ctl; >> + u32 blc_pwm_freq; >> + u32 blc_pwm_duty; >> }; >> >> struct intel_connector { >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index ae808b6..421cd3a 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -816,6 +816,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) >> val &= ~UTIL_PIN_ENABLE; >> I915_WRITE(UTIL_PIN_CTL, val); >> } >> + >> + /* Saving BLC registers for PG0 gating */ >> + panel->blc_pwm_ctl = >> + I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller)); >> + panel->blc_pwm_freq = >> + I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller)); >> + panel->blc_pwm_duty = >> + I915_READ(BXT_BLC_PWM_DUTY(panel->backlight.controller)); >> + >> + >> } >> >> static void pwm_disable_backlight(struct intel_connector *connector) >> @@ -1050,6 +1060,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) >> enum pipe pipe = intel_get_pipe_from_connector(connector); >> u32 pwm_ctl, val; >> >> + /* Restore BLC registers if PG0 was gated */ >> + I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller), >> + panel->blc_pwm_ctl); >> + I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller), >> + panel->blc_pwm_freq); >> + I915_WRITE(BXT_BLC_PWM_DUTY(panel->backlight.controller), >> + panel->blc_pwm_duty); >> + >> /* To use 2nd set of backlight registers, utility pin has to be >> * enabled with PWM mode. >> * The field should only be changed when the utility pin is disabled
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 50f83d2..752fb58 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -193,6 +193,10 @@ struct intel_panel { uint32_t hz); void (*power)(struct intel_connector *, bool enable); } backlight; + + u32 blc_pwm_ctl; + u32 blc_pwm_freq; + u32 blc_pwm_duty; }; struct intel_connector { diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index ae808b6..421cd3a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -816,6 +816,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) val &= ~UTIL_PIN_ENABLE; I915_WRITE(UTIL_PIN_CTL, val); } + + /* Saving BLC registers for PG0 gating */ + panel->blc_pwm_ctl = + I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller)); + panel->blc_pwm_freq = + I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller)); + panel->blc_pwm_duty = + I915_READ(BXT_BLC_PWM_DUTY(panel->backlight.controller)); + + } static void pwm_disable_backlight(struct intel_connector *connector) @@ -1050,6 +1060,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) enum pipe pipe = intel_get_pipe_from_connector(connector); u32 pwm_ctl, val; + /* Restore BLC registers if PG0 was gated */ + I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller), + panel->blc_pwm_ctl); + I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller), + panel->blc_pwm_freq); + I915_WRITE(BXT_BLC_PWM_DUTY(panel->backlight.controller), + panel->blc_pwm_duty); + /* To use 2nd set of backlight registers, utility pin has to be * enabled with PWM mode. * The field should only be changed when the utility pin is disabled