[2/2] drm/i915/psr/cnl: Set y-coordinate as valid in SDP

Message ID	20180330211546.16827-2-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, 30 Mar 2018 14:15:46 -0700 Message-Id: <20180330211546.16827-2-jose.souza@intel.com> In-Reply-To: <20180330211546.16827-1-jose.souza@intel.com> References: <20180330211546.16827-1-jose.souza@intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH 2/2] drm/i915/psr/cnl: Set y-coordinate as valid in SDP Precedence: list Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@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

20180330211546.16827-2-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, 30 Mar 2018 14:15:46 -0700
Message-Id: <20180330211546.16827-2-jose.souza@intel.com>
In-Reply-To: <20180330211546.16827-1-jose.souza@intel.com>
References: <20180330211546.16827-1-jose.souza@intel.com>
MIME-Version: 1.0
Subject: [Intel-gfx] [PATCH 2/2] drm/i915/psr/cnl: Set y-coordinate as valid
	in SDP
Precedence: list
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 30, 2018, 9:15 p.m. UTC

This was my bad, spec says that the name of this bit is
'Y-coordinate valid' but the values for it is:
0: Include Y-coordinate valid eDP1.4a
1: Do not include Y-coordinate valid eDP 1.4
So renaming the bit and not setting it.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 2 +-
 drivers/gpu/drm/i915/intel_psr.c | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Dhinakaran Pandiyan March 30, 2018, 11:46 p.m. UTC | #1

On Fri, 2018-03-30 at 14:15 -0700, José Roberto de Souza wrote:
> This was my bad, spec says that the name of this bit is

> 'Y-coordinate valid' but the values for it is:

> 0: Include Y-coordinate valid eDP1.4a

> 1: Do not include Y-coordinate valid eDP 1.4

> So renaming the bit and not setting it.

> 

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

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

> ---

>  drivers/gpu/drm/i915/i915_reg.h  | 2 +-

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

>  2 files changed, 3 insertions(+), 4 deletions(-)

> 

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

> index 176dca6554f4..19429cb1f3a7 100644

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

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

> @@ -4058,7 +4058,7 @@ enum {

>  #define EDP_PSR2_CTL			_MMIO(0x6f900)

>  #define   EDP_PSR2_ENABLE		(1<<31)

>  #define   EDP_SU_TRACK_ENABLE		(1<<30)

> -#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */

> +#define   EDP_Y_COORDINATE_INVALID	(1<<26) /* GLK and CNL+ */


INVALID isn't the same as asking the source to not send the y-coordinate
valid bit. Sorry to be pedantic, please leave the definition as it is.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if you

remove this hunk.

>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */

>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)

>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)

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

> index 2d53f7398a6d..f12111438bcf 100644

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

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

> @@ -400,9 +400,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)

>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't

>  	 * good enough. */

>  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

> -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {

> -		val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;

> -	}

> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

> +		val |= EDP_Y_COORDINATE_ENABLE;

>  

>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);

>

Souza, Jose March 31, 2018, 12:17 a.m. UTC | #2

On Fri, 2018-03-30 at 16:46 -0700, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-03-30 at 14:15 -0700, José Roberto de Souza wrote:

> > This was my bad, spec says that the name of this bit is

> > 'Y-coordinate valid' but the values for it is:

> > 0: Include Y-coordinate valid eDP1.4a

> > 1: Do not include Y-coordinate valid eDP 1.4

> > So renaming the bit and not setting it.

> > 

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

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

> > ---

> >  drivers/gpu/drm/i915/i915_reg.h  | 2 +-

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

> >  2 files changed, 3 insertions(+), 4 deletions(-)

> > 

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

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

> > index 176dca6554f4..19429cb1f3a7 100644

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

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

> > @@ -4058,7 +4058,7 @@ enum {

> >  #define EDP_PSR2_CTL			_MMIO(0x6f900)

> >  #define   EDP_PSR2_ENABLE		(1<<31)

> >  #define   EDP_SU_TRACK_ENABLE		(1<<30)

> > -#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */

> > +#define   EDP_Y_COORDINATE_INVALID	(1<<26) /* GLK and CNL+

> > */

> 

> INVALID isn't the same as asking the source to not send the y-

> coordinate

> valid bit. Sorry to be pedantic, please leave the definition as it

> is.


I agree with you, if it don't need to send the y-coordinate is
better just not set EDP_Y_COORDINATE_ENABLE but leave the bit
definition name as it is, is prone to others do the same error as I
did.

> 

> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if

> you

> remove this hunk.

> 

> >  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+

> > */

> >  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)

> >  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)

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

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

> > index 2d53f7398a6d..f12111438bcf 100644

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

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

> > @@ -400,9 +400,8 @@ static void hsw_activate_psr2(struct intel_dp

> > *intel_dp)

> >  	 * mesh at all with our frontbuffer tracking. And the hw

> > alone isn't

> >  	 * good enough. */

> >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

> > -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

> > {

> > -		val |= EDP_Y_COORDINATE_VALID |

> > EDP_Y_COORDINATE_ENABLE;

> > -	}

> > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

> > +		val |= EDP_Y_COORDINATE_ENABLE;

> >  

> >  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-

> > >psr.sink_sync_latency + 1);

> >

Dhinakaran Pandiyan April 2, 2018, 5:42 p.m. UTC | #3

On Sat, 2018-03-31 at 00:17 +0000, Souza, Jose wrote:
> On Fri, 2018-03-30 at 16:46 -0700, Pandiyan, Dhinakaran wrote:

> > On Fri, 2018-03-30 at 14:15 -0700, José Roberto de Souza wrote:

> > > This was my bad, spec says that the name of this bit is

> > > 'Y-coordinate valid' but the values for it is:

> > > 0: Include Y-coordinate valid eDP1.4a

> > > 1: Do not include Y-coordinate valid eDP 1.4

> > > So renaming the bit and not setting it.

> > > 

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

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

> > > ---

> > >  drivers/gpu/drm/i915/i915_reg.h  | 2 +-

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

> > >  2 files changed, 3 insertions(+), 4 deletions(-)

> > > 

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

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

> > > index 176dca6554f4..19429cb1f3a7 100644

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

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

> > > @@ -4058,7 +4058,7 @@ enum {

> > >  #define EDP_PSR2_CTL			_MMIO(0x6f900)

> > >  #define   EDP_PSR2_ENABLE		(1<<31)

> > >  #define   EDP_SU_TRACK_ENABLE		(1<<30)

> > > -#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */

> > > +#define   EDP_Y_COORDINATE_INVALID	(1<<26) /* GLK and CNL+

> > > */

> > 

> > INVALID isn't the same as asking the source to not send the y-

> > coordinate

> > valid bit. Sorry to be pedantic, please leave the definition as it

> > is.

> 

> I agree with you, if it don't need to send the y-coordinate is

> better just not set EDP_Y_COORDINATE_ENABLE but leave the bit

> definition name as it is, is prone to others do the same error as I

> did.

> 


I'll let Rodrigo take a call on this one.

-DK

> > 

> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if

> > you

> > remove this hunk.

> > 

> > >  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+

> > > */

> > >  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)

> > >  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)

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

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

> > > index 2d53f7398a6d..f12111438bcf 100644

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

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

> > > @@ -400,9 +400,8 @@ static void hsw_activate_psr2(struct intel_dp

> > > *intel_dp)

> > >  	 * mesh at all with our frontbuffer tracking. And the hw

> > > alone isn't

> > >  	 * good enough. */

> > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

> > > -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

> > > {

> > > -		val |= EDP_Y_COORDINATE_VALID |

> > > EDP_Y_COORDINATE_ENABLE;

> > > -	}

> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

> > > +		val |= EDP_Y_COORDINATE_ENABLE;

> > >  

> > >  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-

> > > >psr.sink_sync_latency + 1);

> > >  

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

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

Rodrigo Vivi April 2, 2018, 6:33 p.m. UTC | #4

On Mon, Apr 02, 2018 at 10:42:23AM -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Sat, 2018-03-31 at 00:17 +0000, Souza, Jose wrote:
> > On Fri, 2018-03-30 at 16:46 -0700, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2018-03-30 at 14:15 -0700, José Roberto de Souza wrote:
> > > > This was my bad, spec says that the name of this bit is
> > > > 'Y-coordinate valid' but the values for it is:
> > > > 0: Include Y-coordinate valid eDP1.4a
> > > > 1: Do not include Y-coordinate valid eDP 1.4
> > > > So renaming the bit and not setting it.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h  | 2 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c | 5 ++---
> > > >  2 files changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 176dca6554f4..19429cb1f3a7 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -4058,7 +4058,7 @@ enum {
> > > >  #define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > >  #define   EDP_PSR2_ENABLE		(1<<31)
> > > >  #define   EDP_SU_TRACK_ENABLE		(1<<30)
> > > > -#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */
> > > > +#define   EDP_Y_COORDINATE_INVALID	(1<<26) /* GLK and CNL+
> > > > */
> > > 
> > > INVALID isn't the same as asking the source to not send the y-
> > > coordinate
> > > valid bit. Sorry to be pedantic, please leave the definition as it
> > > is.
> > 
> > I agree with you, if it don't need to send the y-coordinate is
> > better just not set EDP_Y_COORDINATE_ENABLE but leave the bit
> > definition name as it is, is prone to others do the same error as I
> > did.
> > 
> 
> I'll let Rodrigo take a call on this one.

I agree with DK.

Please use the spec name, i.e. "BitField: Y-coordinate valid"

"invalid" will actually bring even more confusion.

> 
> -DK
> 
> > > 
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if
> > > you
> > > remove this hunk.
> > > 
> > > >  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+
> > > > */
> > > >  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
> > > >  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 2d53f7398a6d..f12111438bcf 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -400,9 +400,8 @@ static void hsw_activate_psr2(struct intel_dp
> > > > *intel_dp)
> > > >  	 * mesh at all with our frontbuffer tracking. And the hw
> > > > alone isn't
> > > >  	 * good enough. */
> > > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > > > -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > > {
> > > > -		val |= EDP_Y_COORDINATE_VALID |
> > > > EDP_Y_COORDINATE_ENABLE;
> > > > -	}
> > > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > > +		val |= EDP_Y_COORDINATE_ENABLE;
> > > >  
> > > >  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-
> > > > >psr.sink_sync_latency + 1);
> > > >  
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 176dca6554f4..19429cb1f3a7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4058,7 +4058,7 @@  enum {
 #define EDP_PSR2_CTL			_MMIO(0x6f900)
 #define   EDP_PSR2_ENABLE		(1<<31)
 #define   EDP_SU_TRACK_ENABLE		(1<<30)
-#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */
+#define   EDP_Y_COORDINATE_INVALID	(1<<26) /* GLK and CNL+ */
 #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
 #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2d53f7398a6d..f12111438bcf 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,9 +400,8 @@  static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
 	 * good enough. */
 	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
-	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
-		val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
-	}
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		val |= EDP_Y_COORDINATE_ENABLE;
 
 	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);

[2/2] drm/i915/psr/cnl: Set y-coordinate as valid in SDP

Commit Message

Comments

Patch