Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore

["Kannan, Vandana" <vandana.kannan@intel.com>]

When I submitted the PPS patch in April, I got an input from Jani to not 
make changes in i915_suspend.c as it was to become obsolete. Below mail 
for your reference.

Jani,
Does your initial comment still hold good?

On Thu, 30 Apr 2015, Vandana Kannan <vandana.kannan@intel.com> wrote:
 > Changes based on future platform readiness patches related to
 > HAS_PCH_SPLIT(). Use HAS_GMCH_DISPLAY() instead of HAS_PCH_SPLIT
 >
 > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
 > Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
 > ---
 >  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
 >  drivers/gpu/drm/i915/intel_dp.c     | 8 ++++----
 >  2 files changed, 6 insertions(+), 6 deletions(-)
 >
 > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
 > b/drivers/gpu/drm/i915/i915_suspend.c
 > index cf67f82..e91d637 100644
 > --- a/drivers/gpu/drm/i915/i915_suspend.c
 > +++ b/drivers/gpu/drm/i915/i915_suspend.c
 > @@ -44,7 +44,7 @@ static void i915_save_display(struct drm_device *dev)
 >  		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
 >
 >  	/* Panel power sequencer */
 > -	if (HAS_PCH_SPLIT(dev)) {
 > +	if (!HAS_GMCH_DISPLAY(dev)) {
 >  		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
 >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
 >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
 > @@ -79,7 +79,7 @@ static void i915_restore_display(struct drm_device 
*dev)
 >  		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
 >
 >  	/* Panel power sequencer */
 > -	if (HAS_PCH_SPLIT(dev)) {
 > +	if (!HAS_GMCH_DISPLAY(dev)) {
 >  		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
 >  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 >  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);

I don't think you should touch i915_suspend.c at all. We're trying to 
get rid of this blind register save/restore, and make everything work in 
the encoder/connector code. Do note that we already skip this for 
vlv/chv power sequencer registers.

 > diff --git a/drivers/gpu/drm/i915/intel_dp.c
 > b/drivers/gpu/drm/i915/intel_dp.c index 937ba31..68e10c1 100644
 > --- a/drivers/gpu/drm/i915/intel_dp.c
 > +++ b/drivers/gpu/drm/i915/intel_dp.c
 > @@ -559,7 +559,7 @@ static u32 _pp_ctrl_reg(struct intel_dp *intel_dp)
 > {
 >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 >
 > -	if (HAS_PCH_SPLIT(dev))
 > +	if (!HAS_GMCH_DISPLAY(dev))
 >  		return PCH_PP_CONTROL;
 >  	else
 >  		return VLV_PIPE_PP_CONTROL(vlv_power_sequencer_pipe(intel_dp));
 > @@ -569,7 +569,7 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
 > {
 >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 >
 > -	if (HAS_PCH_SPLIT(dev))
 > +	if (!HAS_GMCH_DISPLAY(dev))
 >  		return PCH_PP_STATUS;
 >  	else
 >  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
 > @@ -4963,7 +4963,7 @@ intel_dp_init_panel_power_sequencer(struct 
drm_device *dev,
 >  	if (final->t11_t12 != 0)
 >  		return;
 >
 > -	if (HAS_PCH_SPLIT(dev)) {
 > +	if (!HAS_GMCH_DISPLAY(dev)) {
 >  		pp_ctrl_reg = PCH_PP_CONTROL;
 >  		pp_on_reg = PCH_PP_ON_DELAYS;
 >  		pp_off_reg = PCH_PP_OFF_DELAYS;
 > @@ -5063,7 +5063,7 @@
 > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 >
 >  	lockdep_assert_held(&dev_priv->pps_mutex);
 >
 > -	if (HAS_PCH_SPLIT(dev)) {
 > +	if (!HAS_GMCH_DISPLAY(dev)) {
 >  		pp_on_reg = PCH_PP_ON_DELAYS;
 >  		pp_off_reg = PCH_PP_OFF_DELAYS;
 >  		pp_div_reg = PCH_PP_DIVISOR;

For the rest, I'd like you to do what I suggested in my reply to patch 
3, i.e. add a new if (IS_BROXTON(dev)) at the top of each of these, with
BXT_PP_WHATEVER(0) as the register. Later on, we can add code to choose 
the power sequencer, a bit similar to what vlv/chv do now (except there 
it's based on pipe).

BR,
Jani.

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

--
Jani Nikula, Intel Open Source Technology Center

- Vandana


On 12/31/2015 4:26 AM, Matt Roper wrote:
> Broxton-specific panel power sequencing was added in commit
>
>          commit b0a08bec96318be54db97c3f0b9e37b52561f9ea
>          Author: Vandana Kannan <vandana.kannan@intel.com>
>          Date:   Thu Jun 18 11:00:55 2015 +0530
>
>              drm/i915/bxt: eDP Panel Power sequencing
>
> As noted in that commit, Broxton has two sets of registers (and we're
> supposed to read which to use from the VBT, although we're punting on
> that part at the moment) and also doesn't have a divisor register.
> Although we take the BXT-specific details into account at initial system
> init, we fail to save/restore the appropriate registers around system
> suspend/resume, which leads to unclaimed register warnings at the
> moment, and may also fail to save the proper registers once we start
> actually paying attention to the VBT.
>
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_suspend.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index bf582fe..61bec8e 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -44,7 +44,18 @@ static void i915_save_display(struct drm_device *dev)
>   		dev_priv->regfile.saveLVDS = I915_READ(LVDS);
>
>   	/* Panel power sequencer */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (IS_BROXTON(dev)) {
> +		/*
> +		 * TODO: BXT has 2 sets of PPS registers.
> +		 * Correct Register for Broxton need to be identified
> +		 * using VBT. hardcoding for now.
> +		 *
> +		 * There's also no divisor register on Broxton.
> +		 */
> +		dev_priv->regfile.savePP_CONTROL = I915_READ(BXT_PP_CONTROL(0));
> +		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(BXT_PP_ON_DELAYS(0));
> +		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(BXT_PP_OFF_DELAYS(0));
> +	} else if (HAS_PCH_SPLIT(dev)) {
>   		dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
>   		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>   		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
> @@ -83,7 +94,16 @@ static void i915_restore_display(struct drm_device *dev)
>   		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>
>   	/* Panel power sequencer */
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (IS_BROXTON(dev)) {
> +		/*
> +		 * TODO: BXT has 2 sets of PPS registers.
> +		 * Correct Register for Broxton need to be identified
> +		 * using VBT. hardcoding for now
> +		 */
> +		I915_WRITE(BXT_PP_ON_DELAYS(0), dev_priv->regfile.savePP_ON_DELAYS);
> +		I915_WRITE(BXT_PP_OFF_DELAYS(0), dev_priv->regfile.savePP_OFF_DELAYS);
> +		I915_WRITE(BXT_PP_CONTROL(0), dev_priv->regfile.savePP_CONTROL);
> +	} else if (HAS_PCH_SPLIT(dev)) {
>   		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>   		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>   		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx