[PATCH] drm/i915: Avoid spurious WARNs about the wrong pipe in the PPS code

[PATCH] drm/i915: Avoid spurious WARNs about the wrong pipe in the PPS code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Until recently vlv_steal_power_sequencer() wasn't being called for
normal DP ports, and hence it could assert that it should only be
called for pipe A and B (since pipe C doesn't support eDP). However
that changed when we started to consider normal DP ports as well when
choosing a PPS. So we will now get spurious warnings when
vlv_steal_power_sequencer() does get called for pipe C. Avoid this by
moving the WARN down into vlv_detach_power_sequencer() where this
assertion should still hold.

Cc: Imre Deak <imre.deak@intel.com>
Cc: stable@vger.kernel.org
Fixes: 9f2bdb006a7e ("drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV")
References: https://bugs.freedesktop.org/show_bug.cgi?id=95287
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0f14e97e519b..2395046e19ce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2905,6 +2905,9 @@ static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
 
 	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return;
+
 	edp_panel_vdd_off_sync(intel_dp);
 
 	/*
@@ -2932,9 +2935,6 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
-	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
-		return;
-
 	for_each_intel_encoder(dev, encoder) {
 		struct intel_dp *intel_dp;
 		enum port port;
-- 
2.10.2

Re: [PATCH] drm/i915: Avoid spurious WARNs about the wrong pipe in the PPS code

[Imre Deak]

On Wed, Feb 08, 2017 at 07:52:54PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Until recently vlv_steal_power_sequencer() wasn't being called for
> normal DP ports, and hence it could assert that it should only be
> called for pipe A and B (since pipe C doesn't support eDP). However
> that changed when we started to consider normal DP ports as well when
> choosing a PPS. So we will now get spurious warnings when
> vlv_steal_power_sequencer() does get called for pipe C. Avoid this by
> moving the WARN down into vlv_detach_power_sequencer() where this
> assertion should still hold.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 9f2bdb006a7e ("drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV")
> References: https://bugs.freedesktop.org/show_bug.cgi?id=95287
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0f14e97e519b..2395046e19ce 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2905,6 +2905,9 @@ static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
>  
>  	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
>  
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return;
> +

Yep, missed this:
Reviewed-by: Imre Deak <imre.deak@intel.com>

So the above is called only for eDP, checking for that too would make
things clearer imo.

>  	edp_panel_vdd_off_sync(intel_dp);
>  
>  	/*
> @@ -2932,9 +2935,6 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> -	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> -		return;
> -
>  	for_each_intel_encoder(dev, encoder) {
>  		struct intel_dp *intel_dp;
>  		enum port port;
> -- 
> 2.10.2
> 

Re: [PATCH] drm/i915: Avoid spurious WARNs about the wrong pipe in the PPS code

[Ville Syrjälä]

On Thu, Feb 09, 2017 at 12:29:04PM +0200, Imre Deak wrote:
> On Wed, Feb 08, 2017 at 07:52:54PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > Until recently vlv_steal_power_sequencer() wasn't being called for
> > normal DP ports, and hence it could assert that it should only be
> > called for pipe A and B (since pipe C doesn't support eDP). However
> > that changed when we started to consider normal DP ports as well when
> > choosing a PPS. So we will now get spurious warnings when
> > vlv_steal_power_sequencer() does get called for pipe C. Avoid this by
> > moving the WARN down into vlv_detach_power_sequencer() where this
> > assertion should still hold.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 9f2bdb006a7e ("drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV")
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=95287
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 0f14e97e519b..2395046e19ce 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2905,6 +2905,9 @@ static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
> >  
> >  	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
> >  
> > +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> > +		return;
> > +
> 
> Yep, missed this:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> So the above is called only for eDP, checking for that too would make
> things clearer imo.

We should have some is_edp() warns in the even lower level stuff,
I think. Not sure if we should have moved the pipe check even lower as
well. Oh well, at least this should shut up the spurious warns.

Pushed to dinq, thanks for the review.

> 
> >  	edp_panel_vdd_off_sync(intel_dp);
> >  
> >  	/*
> > @@ -2932,9 +2935,6 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > -	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> > -		return;
> > -
> >  	for_each_intel_encoder(dev, encoder) {
> >  		struct intel_dp *intel_dp;
> >  		enum port port;
> > -- 
> > 2.10.2
> > 

-- 
Ville Syrj�l�
Intel OTC