diff mbox series

[v4,7/9] drm/i915: Drop redundant checks to update PSR state

Message ID 20190302013456.24138-7-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/9] drm/i915/psr: Remove PSR2 FIXME | expand

Commit Message

Souza, Jose March 2, 2019, 1:34 a.m. UTC 
All of this checks are redudant and can be removed as the if bellow
already takes care when there is no changes in the state.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Rodrigo Vivi March 4, 2019, 6:42 p.m. UTC | #1 
On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza wrote:
> All of this checks are redudant and can be removed as the if bellow
> already takes care when there is no changes in the state.

is it just redundant or does it really change the behaviour for PSR2
as needed?

code seems right, explanation here not so sure...
but if this is really right and I am missing something feel
free to use:


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

otherwise please change the msg.

Thanks,
Rodrigo.

> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 73453d89a841..d3e3996551c6 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -878,15 +878,11 @@ void intel_psr_update(struct intel_dp *intel_dp,
>  	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
>  		goto unlock;
>  
> -	if (psr->enabled) {
> -		if (!enable || psr2_enable != psr->psr2_enabled)
> -			intel_psr_disable_locked(intel_dp);
> -	}
> +	if (psr->enabled)
> +		intel_psr_disable_locked(intel_dp);
>  
> -	if (enable) {
> -		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
> -			intel_psr_enable_locked(dev_priv, crtc_state);
> -	}
> +	if (enable)
> +		intel_psr_enable_locked(dev_priv, crtc_state);
>  
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan March 4, 2019, 6:54 p.m. UTC | #2 
On Mon, 2019-03-04 at 10:42 -0800, Rodrigo Vivi wrote:
> On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza
> wrote:
> > All of this checks are redudant and can be removed as the if bellow
> > already takes care when there is no changes in the state.
> 
> is it just redundant or does it really change the behaviour for PSR2
> as needed?

I have the same question now that I read José's response to patch 8/9.
At first, I ignored reading this patch because it included the word
"redundant" in the commit message :)

> 
> code seems right, explanation here not so sure...
> but if this is really right and I am missing something feel
> free to use:
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> otherwise please change the msg.
> 
> Thanks,
> Rodrigo.
> 
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 73453d89a841..d3e3996551c6 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -878,15 +878,11 @@ void intel_psr_update(struct intel_dp
> > *intel_dp,
> >  	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> >  		goto unlock;
> >  
> > -	if (psr->enabled) {
> > -		if (!enable || psr2_enable != psr->psr2_enabled)
> > -			intel_psr_disable_locked(intel_dp);
> > -	}
> > +	if (psr->enabled)
> > +		intel_psr_disable_locked(intel_dp);
> >  
> > -	if (enable) {
> > -		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
> > -			intel_psr_enable_locked(dev_priv, crtc_state);
> > -	}
> > +	if (enable)
> > +		intel_psr_enable_locked(dev_priv, crtc_state);
> >  
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> > -- 
> > 2.21.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose March 4, 2019, 8:45 p.m. UTC | #3 
On Mon, 2019-03-04 at 10:54 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2019-03-04 at 10:42 -0800, Rodrigo Vivi wrote:
> > On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza
> > wrote:
> > > All of this checks are redudant and can be removed as the if
> > > bellow
> > > already takes care when there is no changes in the state.
> > 
> > is it just redundant or does it really change the behaviour for
> > PSR2
> > as needed?
> 
> I have the same question now that I read José's response to patch
> 8/9.
> At first, I ignored reading this patch because it included the word
> "redundant" in the commit message :)


It is just redudant, the 'if (enable == psr->enabled && psr2_enable ==
psr->psr2_enabled)' takes care of going from PSR1 -> PSR1, PSR2 -> PSR2
and disabled -> disabled, all other combinations requires one the calls
bellow that can be simplified.


> 
> > code seems right, explanation here not so sure...
> > but if this is really right and I am missing something feel
> > free to use:
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > otherwise please change the msg.
> > 
> > Thanks,
> > Rodrigo.
> > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 73453d89a841..d3e3996551c6 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -878,15 +878,11 @@ void intel_psr_update(struct intel_dp
> > > *intel_dp,
> > >  	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> > >  		goto unlock;
> > >  
> > > -	if (psr->enabled) {
> > > -		if (!enable || psr2_enable != psr->psr2_enabled)
> > > -			intel_psr_disable_locked(intel_dp);
> > > -	}
> > > +	if (psr->enabled)
> > > +		intel_psr_disable_locked(intel_dp);
> > >  
> > > -	if (enable) {
> > > -		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
> > > -			intel_psr_enable_locked(dev_priv, crtc_state);
> > > -	}
> > > +	if (enable)
> > > +		intel_psr_enable_locked(dev_priv, crtc_state);
> > >  
> > >  unlock:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > > -- 
> > > 2.21.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 73453d89a841..d3e3996551c6 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -878,15 +878,11 @@  void intel_psr_update(struct intel_dp *intel_dp,
 	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
 		goto unlock;
 
-	if (psr->enabled) {
-		if (!enable || psr2_enable != psr->psr2_enabled)
-			intel_psr_disable_locked(intel_dp);
-	}
+	if (psr->enabled)
+		intel_psr_disable_locked(intel_dp);
 
-	if (enable) {
-		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
-			intel_psr_enable_locked(dev_priv, crtc_state);
-	}
+	if (enable)
+		intel_psr_enable_locked(dev_priv, crtc_state);
 
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);