diff mbox

[2/4] drm/i915: Remove PSR HW tracking.

Message ID 1409798999-1809-2-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Sept. 4, 2014, 2:49 a.m. UTC
Now that we are tracking psr states on Software side we don't need HW help anymore.
So we can clean up the code a bit and avoid unecessary sets.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ------------
 1 file changed, 12 deletions(-)

Comments

Ville Syrjala Sept. 4, 2014, 8:06 a.m. UTC | #1
On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote:
> Now that we are tracking psr states on Software side we don't need HW help anymore.
> So we can clean up the code a bit and avoid unecessary sets.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a796831..9414e67 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t aux_clock_divider;
> -	int precharge = 0x3;
> -	int msg_size = 5;       /* Header(4) + Message(1) */
>  	bool only_standby = false;
>  
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  	else
>  		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>  				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
> -
> -	/* Setup AUX registers */
> -	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> -	I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
> -	I915_WRITE(EDP_PSR_AUX_CTL(dev),
> -		   DP_AUX_CH_CTL_TIME_OUT_400us |
> -		   (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> -		   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> -		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));

As I said I blieve FBC is the one that does the hw tracking. I think the
hardware will still do the entry/exit (including automagic link
re-train) but now you've not told it how to wake up the sink. So I don't
understand how this can even work.

>  }
>  
>  static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi Sept. 4, 2014, 7:53 p.m. UTC | #2
On Thu, Sep 4, 2014 at 1:06 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> wrote:

> On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote:
> > Now that we are tracking psr states on Software side we don't need HW
> help anymore.
> > So we can clean up the code a bit and avoid unecessary sets.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index a796831..9414e67 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct
> intel_dp *intel_dp)
> >  {
> >       struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >       struct drm_device *dev = dig_port->base.base.dev;
> > -     struct drm_i915_private *dev_priv = dev->dev_private;
> >       uint32_t aux_clock_divider;
> > -     int precharge = 0x3;
> > -     int msg_size = 5;       /* Header(4) + Message(1) */
> >       bool only_standby = false;
> >
> >       aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> > @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct
> intel_dp *intel_dp)
> >       else
> >               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> >                                  DP_PSR_ENABLE |
> DP_PSR_MAIN_LINK_ACTIVE);
> > -
> > -     /* Setup AUX registers */
> > -     I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> > -     I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
> > -     I915_WRITE(EDP_PSR_AUX_CTL(dev),
> > -                DP_AUX_CH_CTL_TIME_OUT_400us |
> > -                (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > -                (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> > -                (aux_clock_divider <<
> DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
>
> As I said I blieve FBC is the one that does the hw tracking. I think the
> hardware will still do the entry/exit (including automagic link
> re-train) but now you've not told it how to wake up the sink. So I don't
> understand how this can even work.
>

Per spec we have to program aux "registers for HW assist PSR Exit Support."

With the exit being made on SW side we don't need HW help anymore.
Same reason why this works with VLV.

But there is no way to claim full control. So automagically triggers still
exist.
So I couldn't remove the W/As that mask events on srd_debug. :(

But withouth these aux sets we are fine and it just works.


>
> >  }
> >
> >  static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Ville Syrjala Sept. 5, 2014, 9:14 a.m. UTC | #3
On Thu, Sep 04, 2014 at 12:53:39PM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 4, 2014 at 1:06 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> > wrote:
> 
> > On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote:
> > > Now that we are tracking psr states on Software side we don't need HW
> > help anymore.
> > > So we can clean up the code a bit and avoid unecessary sets.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 12 ------------
> > >  1 file changed, 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > > index a796831..9414e67 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct
> > intel_dp *intel_dp)
> > >  {
> > >       struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >       struct drm_device *dev = dig_port->base.base.dev;
> > > -     struct drm_i915_private *dev_priv = dev->dev_private;
> > >       uint32_t aux_clock_divider;
> > > -     int precharge = 0x3;
> > > -     int msg_size = 5;       /* Header(4) + Message(1) */
> > >       bool only_standby = false;
> > >
> > >       aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> > > @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct
> > intel_dp *intel_dp)
> > >       else
> > >               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > >                                  DP_PSR_ENABLE |
> > DP_PSR_MAIN_LINK_ACTIVE);
> > > -
> > > -     /* Setup AUX registers */
> > > -     I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> > > -     I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
> > > -     I915_WRITE(EDP_PSR_AUX_CTL(dev),
> > > -                DP_AUX_CH_CTL_TIME_OUT_400us |
> > > -                (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > > -                (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> > > -                (aux_clock_divider <<
> > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
> >
> > As I said I blieve FBC is the one that does the hw tracking. I think the
> > hardware will still do the entry/exit (including automagic link
> > re-train) but now you've not told it how to wake up the sink. So I don't
> > understand how this can even work.
> >
> 
> Per spec we have to program aux "registers for HW assist PSR Exit Support."

And how do you exit w/o HW assist exactly? You never do the DPCD write
manually so I don't see how it can work.

> 
> With the exit being made on SW side we don't need HW help anymore.
> Same reason why this works with VLV.
> 
> But there is no way to claim full control. So automagically triggers still
> exist.
> So I couldn't remove the W/As that mask events on srd_debug. :(
> 
> But withouth these aux sets we are fine and it just works.

Looks like these magic numbers are in fact just the SET_POWER(D0) DPCD
write. Might be good to clarify this with Art, but I think what is happening
is that you're using link standby, so just the PSR SDP sent inline on the
main link is enough to exit PSR. But if you'd use the main link off mode
then you'd need this AUX stuff.

We should probably kill the magic numbers here and just assemble the AUX
data like we would do for normal AUX. At least that way people would
understand what it's meant to do from just reading the code. Right now
it looks like black magic and the define names don't really help either.

> 
> 
> >
> > >  }
> > >
> > >  static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> > > --
> > > 1.9.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a796831..9414e67 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1778,10 +1778,7 @@  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t aux_clock_divider;
-	int precharge = 0x3;
-	int msg_size = 5;       /* Header(4) + Message(1) */
 	bool only_standby = false;
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
@@ -1796,15 +1793,6 @@  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 	else
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
 				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
-
-	/* Setup AUX registers */
-	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
-	I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
-	I915_WRITE(EDP_PSR_AUX_CTL(dev),
-		   DP_AUX_CH_CTL_TIME_OUT_400us |
-		   (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
-		   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
-		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
 }
 
 static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)