[v2,5/5] drm/i915/psr: Simply PSR computed state

Message ID	20180316230501.974-5-jose.souza@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: =?UTF-8?q?Jos=C3=A9=20Roberto=20de=20Souza?= <jose.souza@intel.com> To: intel-gfx@lists.freedesktop.org Date: Fri, 16 Mar 2018 16:05:01 -0700 Message-Id: <20180316230501.974-5-jose.souza@intel.com> In-Reply-To: <20180316230501.974-1-jose.souza@intel.com> References: <20180316230501.974-1-jose.souza@intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH v2 5/5] drm/i915/psr: Simply PSR computed state Precedence: list Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>, Rodrigo Vivi <rodrigo.vivi@intel.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Message ID

20180316230501.974-5-jose.souza@intel.com (mailing list archive)

State

New, archived

Headers

From: =?UTF-8?q?Jos=C3=A9=20Roberto=20de=20Souza?= <jose.souza@intel.com>
To: intel-gfx@lists.freedesktop.org
Date: Fri, 16 Mar 2018 16:05:01 -0700
Message-Id: <20180316230501.974-5-jose.souza@intel.com>
In-Reply-To: <20180316230501.974-1-jose.souza@intel.com>
References: <20180316230501.974-1-jose.souza@intel.com>
MIME-Version: 1.0
Subject: [Intel-gfx] [PATCH v2 5/5] drm/i915/psr: Simply PSR computed state
Precedence: list
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Commit Message

Souza, Jose March 16, 2018, 11:05 p.m. UTC

Having has_psr and has_psr2 can be ambiguous and also uses one more
byte than needed(not taking in care struct alignment).

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.

 drivers/gpu/drm/i915/intel_drv.h |  3 +--
 drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

Comments

Rodrigo Vivi March 16, 2018, 11:30 p.m. UTC | #1

On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza wrote:
> Having has_psr and has_psr2 can be ambiguous and also uses one more
> byte than needed(not taking in care struct alignment).
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.
> 
>  drivers/gpu/drm/i915/intel_drv.h |  3 +--
>  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a215aa78b0be..a7383235f90a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -807,8 +807,7 @@ struct intel_crtc_state {
>  	struct intel_link_m_n dp_m2_n2;
>  	bool has_drrs;
>  
> -	bool has_psr;
> -	bool has_psr2;
> +	u8 psr;
>  
>  	/*
>  	 * Frequence the dpll for the port should run at. Differs from the
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index aa4e03f65386..78b5c0c88261 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -563,9 +563,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> -	crtc_state->has_psr = true;
> -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> -	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
> +	if (intel_psr2_config_valid(intel_dp, crtc_state))
> +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;
> +	else
> +		crtc_state->psr = DP_PSR_IS_SUPPORTED;
> +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);

Could we still continue writing "PSR" instead of "PSR1" ?

otherwise patch lgtm...

>  }
>  
>  static void intel_psr_activate(struct intel_dp *intel_dp)
> @@ -635,7 +637,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	if (!crtc_state->has_psr)
> +	if (!crtc_state->psr)
>  		return;
>  
>  	if (WARN_ON(!CAN_PSR(dev_priv)))
> @@ -648,7 +650,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  		goto unlock;
>  	}
>  
> -	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
> +	dev_priv->psr.psr2_enabled = (crtc_state->psr == DP_PSR2_IS_SUPPORTED);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  
>  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> @@ -770,7 +772,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	if (!old_crtc_state->has_psr)
> +	if (!old_crtc_state->psr)
>  		return;
>  
>  	if (WARN_ON(!CAN_PSR(dev_priv)))
> -- 
> 2.16.2
>

Dhinakaran Pandiyan March 17, 2018, 12:38 a.m. UTC | #2

On Fri, 2018-03-16 at 16:30 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza wrote:

> > Having has_psr and has_psr2 can be ambiguous and also uses one more

> > byte than needed(not taking in care struct alignment).

> > 

> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> > ---

> > 

> > v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran Pandiyan.

> > 

> >  drivers/gpu/drm/i915/intel_drv.h |  3 +--

> >  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------

> >  2 files changed, 9 insertions(+), 8 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h

> > index a215aa78b0be..a7383235f90a 100644

> > --- a/drivers/gpu/drm/i915/intel_drv.h

> > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > @@ -807,8 +807,7 @@ struct intel_crtc_state {

> >  	struct intel_link_m_n dp_m2_n2;

> >  	bool has_drrs;

> >  

> > -	bool has_psr;

> > -	bool has_psr2;

> > +	u8 psr;


/* 0 = disabled, 1 = PSR1, 2 = PSR2 */

> >  

> >  	/*

> >  	 * Frequence the dpll for the port should run at. Differs from the

> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c

> > index aa4e03f65386..78b5c0c88261 100644

> > --- a/drivers/gpu/drm/i915/intel_psr.c

> > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > @@ -563,9 +563,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,

> >  		return;

> >  	}

> >  

> > -	crtc_state->has_psr = true;

> > -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);

> > -	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");

> > +	if (intel_psr2_config_valid(intel_dp, crtc_state))

> > +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;


We can avoid the dependency on an unrelated macro definition if you
explicitly set it to 1 or 2.

> > +	else

> > +		crtc_state->psr = DP_PSR_IS_SUPPORTED;

> > +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);


Also I think you should initialize ->psr = 0
> 

> Could we still continue writing "PSR" instead of "PSR1" ?

> 

> otherwise patch lgtm...

> 

> >  }

> >  

> >  static void intel_psr_activate(struct intel_dp *intel_dp)

> > @@ -635,7 +637,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,

> >  	struct drm_device *dev = intel_dig_port->base.base.dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  

> > -	if (!crtc_state->has_psr)

> > +	if (!crtc_state->psr)

> >  		return;

> >  

> >  	if (WARN_ON(!CAN_PSR(dev_priv)))

> > @@ -648,7 +650,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,

> >  		goto unlock;

> >  	}

> >  

> > -	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;

> > +	dev_priv->psr.psr2_enabled = (crtc_state->psr == DP_PSR2_IS_SUPPORTED);

> >  	dev_priv->psr.busy_frontbuffer_bits = 0;

> >  

> >  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);

> > @@ -770,7 +772,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,

> >  	struct drm_device *dev = intel_dig_port->base.base.dev;

> >  	struct drm_i915_private *dev_priv = to_i915(dev);

> >  

> > -	if (!old_crtc_state->has_psr)

> > +	if (!old_crtc_state->psr)

> >  		return;

> >  

> >  	if (WARN_ON(!CAN_PSR(dev_priv)))

> > -- 

> > 2.16.2

> > 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Souza, Jose March 17, 2018, 1:31 a.m. UTC | #3

On Fri, 2018-03-16 at 17:38 -0700, Pandiyan, Dhinakaran wrote:
> 

> 

> On Fri, 2018-03-16 at 16:30 -0700, Rodrigo Vivi wrote:

> > On Fri, Mar 16, 2018 at 04:05:01PM -0700, José Roberto de Souza

> > wrote:

> > > Having has_psr and has_psr2 can be ambiguous and also uses one

> > > more

> > > byte than needed(not taking in care struct alignment).

> > > 

> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> > > ---

> > > 

> > > v2: Grouped the 2 bools into one u8 as suggested by Dhinakaran

> > > Pandiyan.

> > > 

> > >  drivers/gpu/drm/i915/intel_drv.h |  3 +--

> > >  drivers/gpu/drm/i915/intel_psr.c | 14 ++++++++------

> > >  2 files changed, 9 insertions(+), 8 deletions(-)

> > > 

> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h

> > > b/drivers/gpu/drm/i915/intel_drv.h

> > > index a215aa78b0be..a7383235f90a 100644

> > > --- a/drivers/gpu/drm/i915/intel_drv.h

> > > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > > @@ -807,8 +807,7 @@ struct intel_crtc_state {

> > >  	struct intel_link_m_n dp_m2_n2;

> > >  	bool has_drrs;

> > >  

> > > -	bool has_psr;

> > > -	bool has_psr2;

> > > +	u8 psr;

> 

> /* 0 = disabled, 1 = PSR1, 2 = PSR2 */

> 

> > >  

> > >  	/*

> > >  	 * Frequence the dpll for the port should run at.

> > > Differs from the

> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c

> > > b/drivers/gpu/drm/i915/intel_psr.c

> > > index aa4e03f65386..78b5c0c88261 100644

> > > --- a/drivers/gpu/drm/i915/intel_psr.c

> > > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > > @@ -563,9 +563,11 @@ void intel_psr_compute_config(struct

> > > intel_dp *intel_dp,

> > >  		return;

> > >  	}

> > >  

> > > -	crtc_state->has_psr = true;

> > > -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,

> > > crtc_state);

> > > -	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ?

> > > "2" : "");

> > > +	if (intel_psr2_config_valid(intel_dp, crtc_state))

> > > +		crtc_state->psr = DP_PSR2_IS_SUPPORTED;

> 

> We can avoid the dependency on an unrelated macro definition if you

> explicitly set it to 1 or 2.


I don't like the idea of using magic numbers, what do you think about
add enum?

> 

> > > +	else

> > > +		crtc_state->psr = DP_PSR_IS_SUPPORTED;

> > > +	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);

> 

> Also I think you should initialize ->psr = 0

> > 

> > Could we still continue writing "PSR" instead of "PSR1" ?

> > 

> > otherwise patch lgtm...

> > 

> > >  }

> > >  

> > >  static void intel_psr_activate(struct intel_dp *intel_dp)

> > > @@ -635,7 +637,7 @@ void intel_psr_enable(struct intel_dp

> > > *intel_dp,

> > >  	struct drm_device *dev = intel_dig_port->base.base.dev;

> > >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > >  

> > > -	if (!crtc_state->has_psr)

> > > +	if (!crtc_state->psr)

> > >  		return;

> > >  

> > >  	if (WARN_ON(!CAN_PSR(dev_priv)))

> > > @@ -648,7 +650,7 @@ void intel_psr_enable(struct intel_dp

> > > *intel_dp,

> > >  		goto unlock;

> > >  	}

> > >  

> > > -	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;

> > > +	dev_priv->psr.psr2_enabled = (crtc_state->psr ==

> > > DP_PSR2_IS_SUPPORTED);

> > >  	dev_priv->psr.busy_frontbuffer_bits = 0;

> > >  

> > >  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);

> > > @@ -770,7 +772,7 @@ void intel_psr_disable(struct intel_dp

> > > *intel_dp,

> > >  	struct drm_device *dev = intel_dig_port->base.base.dev;

> > >  	struct drm_i915_private *dev_priv = to_i915(dev);

> > >  

> > > -	if (!old_crtc_state->has_psr)

> > > +	if (!old_crtc_state->psr)

> > >  		return;

> > >  

> > >  	if (WARN_ON(!CAN_PSR(dev_priv)))

> > > -- 

> > > 2.16.2

> > > 

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a215aa78b0be..a7383235f90a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -807,8 +807,7 @@  struct intel_crtc_state {
 	struct intel_link_m_n dp_m2_n2;
 	bool has_drrs;
 
-	bool has_psr;
-	bool has_psr2;
+	u8 psr;
 
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aa4e03f65386..78b5c0c88261 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -563,9 +563,11 @@  void intel_psr_compute_config(struct intel_dp *intel_dp,
 		return;
 	}
 
-	crtc_state->has_psr = true;
-	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
-	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
+	if (intel_psr2_config_valid(intel_dp, crtc_state))
+		crtc_state->psr = DP_PSR2_IS_SUPPORTED;
+	else
+		crtc_state->psr = DP_PSR_IS_SUPPORTED;
+	DRM_DEBUG_KMS("Enabling PSR%d\n", crtc_state->psr);
 }
 
 static void intel_psr_activate(struct intel_dp *intel_dp)
@@ -635,7 +637,7 @@  void intel_psr_enable(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!crtc_state->has_psr)
+	if (!crtc_state->psr)
 		return;
 
 	if (WARN_ON(!CAN_PSR(dev_priv)))
@@ -648,7 +650,7 @@  void intel_psr_enable(struct intel_dp *intel_dp,
 		goto unlock;
 	}
 
-	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
+	dev_priv->psr.psr2_enabled = (crtc_state->psr == DP_PSR2_IS_SUPPORTED);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
@@ -770,7 +772,7 @@  void intel_psr_disable(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (!old_crtc_state->has_psr)
+	if (!old_crtc_state->psr)
 		return;
 
 	if (WARN_ON(!CAN_PSR(dev_priv)))

[v2,5/5] drm/i915/psr: Simply PSR computed state

Commit Message

Comments

Patch