[PATCH 01/12] drm: Add DP PSR2 sink enable bit

[PATCH 01/12] drm: Add DP PSR2 sink enable bit

[José Roberto de Souza]

To comply with eDP1.4a this bit should be set when enabling PSR2.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 include/drm/drm_dp_helper.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 62903bae0221..0bac0c7d0dec 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -478,6 +478,7 @@
 # define DP_PSR_FRAME_CAPTURE		    (1 << 3)
 # define DP_PSR_SELECTIVE_UPDATE	    (1 << 4)
 # define DP_PSR_IRQ_HPD_WITH_CRC_ERRORS     (1 << 5)
+# define DP_PSR_ENABLE_PSR2		    (1 << 6) /* eDP 1.4a */
 
 #define DP_ADAPTER_CTRL			    0x1a0
 # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE   (1 << 0)
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/12] drm: Add DP last received PSR SDP VSC register and bits

[José Roberto de Souza]

This is a register to help debug what is in the last SDP VSC
packet revived by sink.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 include/drm/drm_dp_helper.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 0bac0c7d0dec..91c9bcd4196f 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -795,6 +795,15 @@
 # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_MASK	(0xf << 4)
 # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_SHIFT	4
 
+#define DP_LAST_RECEIVED_PSR_SDP	    0x200a /* eDP 1.2 */
+# define DP_PSR_STATE_BIT		    (1 << 0) /* eDP 1.2 */
+# define DP_UPDATE_RFB_BIT		    (1 << 1) /* eDP 1.2 */
+# define DP_CRC_VALID_BIT		    (1 << 2) /* eDP 1.2 */
+# define DP_SU_VALID			    (1 << 3) /* eDP 1.4 */
+# define DP_FIRST_SCAN_LINE_SU_REGION	    (1 << 4) /* eDP 1.4 */
+# define DP_LAST_SCAN_LINE_SU_REGION	    (1 << 5) /* eDP 1.4 */
+# define DP_Y_COORDINATE_VALID		    (1 << 6) /* eDP 1.4a */
+
 #define DP_RECEIVER_ALPM_STATUS		    0x200b  /* eDP 1.4 */
 # define DP_ALPM_LOCK_TIMEOUT_ERROR	    (1 << 0)
 
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm: Add DP last received PSR SDP VSC register and bits

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:38PM -0700, José Roberto de Souza wrote:
> This is a register to help debug what is in the last SDP VSC
> packet revived by sink.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>


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

(just looking to 1.4b one, but the versions on the comments seems right)

> ---
>  include/drm/drm_dp_helper.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 0bac0c7d0dec..91c9bcd4196f 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -795,6 +795,15 @@
>  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_MASK	(0xf << 4)
>  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_SHIFT	4
>  
> +#define DP_LAST_RECEIVED_PSR_SDP	    0x200a /* eDP 1.2 */
> +# define DP_PSR_STATE_BIT		    (1 << 0) /* eDP 1.2 */
> +# define DP_UPDATE_RFB_BIT		    (1 << 1) /* eDP 1.2 */
> +# define DP_CRC_VALID_BIT		    (1 << 2) /* eDP 1.2 */
> +# define DP_SU_VALID			    (1 << 3) /* eDP 1.4 */
> +# define DP_FIRST_SCAN_LINE_SU_REGION	    (1 << 4) /* eDP 1.4 */
> +# define DP_LAST_SCAN_LINE_SU_REGION	    (1 << 5) /* eDP 1.4 */
> +# define DP_Y_COORDINATE_VALID		    (1 << 6) /* eDP 1.4a */
> +
>  #define DP_RECEIVER_ALPM_STATUS		    0x200b  /* eDP 1.4 */
>  # define DP_ALPM_LOCK_TIMEOUT_ERROR	    (1 << 0)
>  
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm: Add DP last received PSR SDP VSC register and bits

[Souza, Jose]

On Thu, 2018-03-22 at 16:23 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:38PM -0700, José Roberto de Souza
> wrote:
> > This is a register to help debug what is in the last SDP VSC
> > packet revived by sink.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> (just looking to 1.4b one, but the versions on the comments seems
> right)

Thanks, I had sent this 2 ones to dri-devel directly, better give the
Reviewed-by over there? https://patchwork.freedesktop.org/series/40512/

> 
> > ---
> >  include/drm/drm_dp_helper.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/include/drm/drm_dp_helper.h
> > b/include/drm/drm_dp_helper.h
> > index 0bac0c7d0dec..91c9bcd4196f 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -795,6 +795,15 @@
> >  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_MASK	(0xf
> > << 4)
> >  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_SHIFT	4
> >  
> > +#define DP_LAST_RECEIVED_PSR_SDP	    0x200a /* eDP 1.2 */
> > +# define DP_PSR_STATE_BIT		    (1 << 0) /* eDP 1.2
> > */
> > +# define DP_UPDATE_RFB_BIT		    (1 << 1) /* eDP 1.2
> > */
> > +# define DP_CRC_VALID_BIT		    (1 << 2) /* eDP 1.2
> > */
> > +# define DP_SU_VALID			    (1 << 3) /* eDP
> > 1.4 */
> > +# define DP_FIRST_SCAN_LINE_SU_REGION	    (1 << 4) /* eDP
> > 1.4 */
> > +# define DP_LAST_SCAN_LINE_SU_REGION	    (1 << 5) /* eDP
> > 1.4 */
> > +# define DP_Y_COORDINATE_VALID		    (1 << 6) /* eDP
> > 1.4a */
> > +
> >  #define DP_RECEIVER_ALPM_STATUS		    0x200b  /* eDP
> > 1.4 */
> >  # define DP_ALPM_LOCK_TIMEOUT_ERROR	    (1 << 0)
> >  
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm: Add DP last received PSR SDP VSC register and bits

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 05:59:16PM -0700, Souza, Jose wrote:
> On Thu, 2018-03-22 at 16:23 -0700, Rodrigo Vivi wrote:
> > On Thu, Mar 22, 2018 at 02:48:38PM -0700, José Roberto de Souza
> > wrote:
> > > This is a register to help debug what is in the last SDP VSC
> > > packet revived by sink.
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > (just looking to 1.4b one, but the versions on the comments seems
> > right)
> 
> Thanks, I had sent this 2 ones to dri-devel directly, better give the
> Reviewed-by over there? https://patchwork.freedesktop.org/series/40512/

Oh, I hadn't noticed they were disconnected. On cases like you could have
cc'ed dri-devel on the series. So it gets clear when we ask for drm maintainer
ack to merge on drm-intel that we have dependency on that patch.

Right now I think you can split this series with the already reviewed-by
series. Include my rv-b on these first two already and resend it here
cc'ing dri-devel. So we ask ack there, and get ci results and already
push those to dinq.

Thanks,
Rodrigo.

> 
> > 
> > > ---
> > >  include/drm/drm_dp_helper.h | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/include/drm/drm_dp_helper.h
> > > b/include/drm/drm_dp_helper.h
> > > index 0bac0c7d0dec..91c9bcd4196f 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -795,6 +795,15 @@
> > >  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_MASK	(0xf
> > > << 4)
> > >  # define DP_LAST_ACTUAL_SYNCHRONIZATION_LATENCY_SHIFT	4
> > >  
> > > +#define DP_LAST_RECEIVED_PSR_SDP	    0x200a /* eDP 1.2 */
> > > +# define DP_PSR_STATE_BIT		    (1 << 0) /* eDP 1.2
> > > */
> > > +# define DP_UPDATE_RFB_BIT		    (1 << 1) /* eDP 1.2
> > > */
> > > +# define DP_CRC_VALID_BIT		    (1 << 2) /* eDP 1.2
> > > */
> > > +# define DP_SU_VALID			    (1 << 3) /* eDP
> > > 1.4 */
> > > +# define DP_FIRST_SCAN_LINE_SU_REGION	    (1 << 4) /* eDP
> > > 1.4 */
> > > +# define DP_LAST_SCAN_LINE_SU_REGION	    (1 << 5) /* eDP
> > > 1.4 */
> > > +# define DP_Y_COORDINATE_VALID		    (1 << 6) /* eDP
> > > 1.4a */
> > > +
> > >  #define DP_RECEIVER_ALPM_STATUS		    0x200b  /* eDP
> > > 1.4 */
> > >  # define DP_ALPM_LOCK_TIMEOUT_ERROR	    (1 << 0)
> > >  
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[José Roberto de Souza]

Without GTC enabled hardware is sending dummy aux frame sync value
that is not useful to sink do selective update, that is why it also
require that sink supports and requires the y-coordinate.

So removing everything related to aux frame sync, if GTC is enabled
we can bring this back.

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>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
 2 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9c3b2ba6a86..7fe00509e51a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -602,7 +602,6 @@ struct i915_psr {
 	struct delayed_work work;
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
-	bool aux_frame_sync;
 	bool link_standby;
 	bool y_cord_support;
 	bool colorimetry_support;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b8e083e10029..d46320a451d9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 
 	if (INTEL_GEN(dev_priv) >= 9 &&
 	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
-		uint8_t frame_sync_cap;
 
 		dev_priv->psr.sink_support = true;
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-				      &frame_sync_cap) != 1)
-			frame_sync_cap = 0;
-		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
-		/* PSR2 needs frame sync as well */
-		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
+		dev_priv->psr.psr2_support = true;
 		DRM_DEBUG_KMS("PSR2 %s on sink",
 			      dev_priv->psr.psr2_support ? "supported" : "not supported");
 
@@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 
-	/* Enable AUX frame sync at sink */
-	if (dev_priv->psr.aux_frame_sync)
-		drm_dp_dpcd_writeb(&intel_dp->aux,
-				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
-				DP_AUX_FRAME_SYNC_ENABLE);
 	/* Enable ALPM at sink for psr2 */
 	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
@@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
-		if (dev_priv->psr.aux_frame_sync)
-			drm_dp_dpcd_writeb(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
-					0);
-
 		if (dev_priv->psr.psr2_support) {
 			psr_status = EDP_PSR2_STATUS;
 			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
@@ -860,10 +843,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.aux_frame_sync)
-			drm_dp_dpcd_writeb(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
-					0);
 		if (dev_priv->psr.psr2_support) {
 			val = I915_READ(EDP_PSR2_CTL);
 			WARN_ON(!(val & EDP_PSR2_ENABLE));
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza wrote:
> Without GTC enabled hardware is sending dummy aux frame sync value
> that is not useful to sink do selective update, that is why it also
> require that sink supports and requires the y-coordinate.
> 
> So removing everything related to aux frame sync, if GTC is enabled
> we can bring this back.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

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

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(but I would like to give a time for Vathsala to comment on this)

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
>  2 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9c3b2ba6a86..7fe00509e51a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -602,7 +602,6 @@ struct i915_psr {
>  	struct delayed_work work;
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
> -	bool aux_frame_sync;
>  	bool link_standby;
>  	bool y_cord_support;
>  	bool colorimetry_support;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b8e083e10029..d46320a451d9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
>  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> -		uint8_t frame_sync_cap;
>  
>  		dev_priv->psr.sink_support = true;
> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> -				      &frame_sync_cap) != 1)
> -			frame_sync_cap = 0;
> -		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
> -		/* PSR2 needs frame sync as well */
> -		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> +		dev_priv->psr.psr2_support = true;
>  		DRM_DEBUG_KMS("PSR2 %s on sink",
>  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>  
> @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  
> -	/* Enable AUX frame sync at sink */
> -	if (dev_priv->psr.aux_frame_sync)
> -		drm_dp_dpcd_writeb(&intel_dp->aux,
> -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -				DP_AUX_FRAME_SYNC_ENABLE);
>  	/* Enable ALPM at sink for psr2 */
>  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
> @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>  		i915_reg_t psr_status;
>  		u32 psr_status_mask;
>  
> -		if (dev_priv->psr.aux_frame_sync)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
> -
>  		if (dev_priv->psr.psr2_support) {
>  			psr_status = EDP_PSR2_STATUS;
>  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> @@ -860,10 +843,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.aux_frame_sync)
> -			drm_dp_dpcd_writeb(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> -					0);
>  		if (dev_priv->psr.psr2_support) {
>  			val = I915_READ(EDP_PSR2_CTL);
>  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Souza, Jose]

On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza
> wrote:
> > Without GTC enabled hardware is sending dummy aux frame sync value
> > that is not useful to sink do selective update, that is why it also
> > require that sink supports and requires the y-coordinate.
> > 
> > So removing everything related to aux frame sync, if GTC is enabled
> > we can bring this back.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> (but I would like to give a time for Vathsala to comment on this)

Okay, I will wait Vathsala comments to send another version.
Thanks

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
> >  2 files changed, 1 insertion(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index c9c3b2ba6a86..7fe00509e51a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -602,7 +602,6 @@ struct i915_psr {
> >  	struct delayed_work work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> > -	bool aux_frame_sync;
> >  	bool link_standby;
> >  	bool y_cord_support;
> >  	bool colorimetry_support;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index b8e083e10029..d46320a451d9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > -		uint8_t frame_sync_cap;
> >  
> >  		dev_priv->psr.sink_support = true;
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_SINK_DEVICE_AUX_FRAME_SYN
> > C_CAP,
> > -				      &frame_sync_cap) != 1)
> > -			frame_sync_cap = 0;
> > -		dev_priv->psr.aux_frame_sync = frame_sync_cap &
> > DP_AUX_FRAME_SYNC_CAP;
> > -		/* PSR2 needs frame sync as well */
> > -		dev_priv->psr.psr2_support = dev_priv-
> > >psr.aux_frame_sync;
> > +		dev_priv->psr.psr2_support = true;
> >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> >  			      dev_priv->psr.psr2_support ?
> > "supported" : "not supported");
> >  
> > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct
> > intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  
> > -	/* Enable AUX frame sync at sink */
> > -	if (dev_priv->psr.aux_frame_sync)
> > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF
> > ,
> > -				DP_AUX_FRAME_SYNC_ENABLE);
> >  	/* Enable ALPM at sink for psr2 */
> >  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> >  		i915_reg_t psr_status;
> >  		u32 psr_status_mask;
> >  
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_S
> > YNC_CONF,
> > -					0);
> > -
> >  		if (dev_priv->psr.psr2_support) {
> >  			psr_status = EDP_PSR2_STATUS;
> >  			psr_status_mask =
> > EDP_PSR2_STATUS_STATE_MASK;
> > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct
> > drm_i915_private *dev_priv)
> >  		return;
> >  
> >  	if (HAS_DDI(dev_priv)) {
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_S
> > YNC_CONF,
> > -					0);
> >  		if (dev_priv->psr.psr2_support) {
> >  			val = I915_READ(EDP_PSR2_CTL);
> >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Pandiyan, Dhinakaran]

On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza wrote:
> > Without GTC enabled hardware is sending dummy aux frame sync value

Curious, is this something you found by testing?

> > that is not useful to sink do selective update, that is why it also
> > require that sink supports and requires the y-coordinate.
> > 
> > So removing everything related to aux frame sync, if GTC is enabled
> > we can bring this back.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> (but I would like to give a time for Vathsala to comment on this)
> 

Reading the spec again,

No aux frame sync implies that the driver does not support selective
update, irrespective of whether we *support y-coordinate or not*.

Section 7.1 eDP v1.4b
"AUX_FRAME_SYNC was added to eDP v1.4 for streamlining the RFB
management of a Sink device during PSR2 operation. AUX_FRAME_SYNC is
required a for PSR2 when using the Selective Update feature (which is
any time that the SU_VALID bit p is set to 1 in the PSR2 VSC SDP).
Neither GTC nor AUX_FRAME_SYNC is required for PSR2 when using only the
Single-frame Update feature (which means that the SU_VALID bit is always
cleared to 0 in the PSR2 VSC SDP)."

Sending y-coordinate only allows for a lower precision aux frame sync
functionality, doesn't invalidate the need for aux frame sync.

"When the Source device includes the optional Y-coordinate in the SDP
for PSR2 Operation, as described in Table 6-12, the Sink device can
implement a lower-precision GTC Slave  function, as described in Table
7-1."

Aside from the still slightly misleading commit message, this in my
opinion is a good step. Let's make it obvious and clear that only full
frame updates are currently supported. We can then start working on
what's required for selective update and do it properly.


I'd like you to include relevant portions of the above text in the
commit message if you agree with the interpretation.

-DK



> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
> >  2 files changed, 1 insertion(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c9c3b2ba6a86..7fe00509e51a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -602,7 +602,6 @@ struct i915_psr {
> >  	struct delayed_work work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> > -	bool aux_frame_sync;
> >  	bool link_standby;
> >  	bool y_cord_support;
> >  	bool colorimetry_support;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index b8e083e10029..d46320a451d9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > -		uint8_t frame_sync_cap;
> >  
> >  		dev_priv->psr.sink_support = true;
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -				      &frame_sync_cap) != 1)
> > -			frame_sync_cap = 0;
> > -		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
> > -		/* PSR2 needs frame sync as well */
> > -		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > +		dev_priv->psr.psr2_support = true;
> >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> >  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> >  
> > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  
> > -	/* Enable AUX frame sync at sink */
> > -	if (dev_priv->psr.aux_frame_sync)
> > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -				DP_AUX_FRAME_SYNC_ENABLE);
> >  	/* Enable ALPM at sink for psr2 */
> >  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> >  		i915_reg_t psr_status;
> >  		u32 psr_status_mask;
> >  
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -					0);
> > -
> >  		if (dev_priv->psr.psr2_support) {
> >  			psr_status = EDP_PSR2_STATUS;
> >  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
> >  		return;
> >  
> >  	if (HAS_DDI(dev_priv)) {
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -					0);
> >  		if (dev_priv->psr.psr2_support) {
> >  			val = I915_READ(EDP_PSR2_CTL);
> >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Souza, Jose]

On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> > On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza
> > wrote:
> > > Without GTC enabled hardware is sending dummy aux frame sync
> > > value
> 
> Curious, is this something you found by testing?

There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE
register in sink, I'm reading this as valid but the value of aux frame
in sink is always 0, the same as GTC_LIVE register in source side by
this I guess source is sending 0 in each aux frame sync.

> 
> > > that is not useful to sink do selective update, that is why it
> > > also
> > > require that sink supports and requires the y-coordinate.
> > > 
> > > So removing everything related to aux frame sync, if GTC is
> > > enabled
> > > we can bring this back.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > (but I would like to give a time for Vathsala to comment on this)
> > 
> 
> Reading the spec again,
> 
> No aux frame sync implies that the driver does not support selective
> update, irrespective of whether we *support y-coordinate or not*.
> 
> Section 7.1 eDP v1.4b
> "AUX_FRAME_SYNC was added to eDP v1.4 for streamlining the RFB
> management of a Sink device during PSR2 operation. AUX_FRAME_SYNC is
> required a for PSR2 when using the Selective Update feature (which is
> any time that the SU_VALID bit p is set to 1 in the PSR2 VSC SDP).
> Neither GTC nor AUX_FRAME_SYNC is required for PSR2 when using only
> the
> Single-frame Update feature (which means that the SU_VALID bit is
> always
> cleared to 0 in the PSR2 VSC SDP)."
> 
> Sending y-coordinate only allows for a lower precision aux frame sync
> functionality, doesn't invalidate the need for aux frame sync.
> 
> "When the Source device includes the optional Y-coordinate in the SDP
> for PSR2 Operation, as described in Table 6-12, the Sink device can
> implement a lower-precision GTC Slave  function, as described in
> Table
> 7-1."
> 
> Aside from the still slightly misleading commit message, this in my
> opinion is a good step. Let's make it obvious and clear that only
> full
> frame updates are currently supported. We can then start working on
> what's required for selective update and do it properly.
> 
> 
> I'd like you to include relevant portions of the above text in the
> commit message if you agree with the interpretation.
> 
> -DK

I agree with your interpretation of the spec but by the previous
attempts of enabling of PSR/PSR2 and also by my tests the selective
update with y-coordinate works at least in the pannels that we have
access to.
And do not makes sense enable any PSR2 if we are going to do full frame
updates, that is PSR1.

> 
> 
> 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > >  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
> > >  2 files changed, 1 insertion(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index c9c3b2ba6a86..7fe00509e51a 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -602,7 +602,6 @@ struct i915_psr {
> > >  	struct delayed_work work;
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool psr2_support;
> > > -	bool aux_frame_sync;
> > >  	bool link_standby;
> > >  	bool y_cord_support;
> > >  	bool colorimetry_support;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index b8e083e10029..d46320a451d9 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > -		uint8_t frame_sync_cap;
> > >  
> > >  		dev_priv->psr.sink_support = true;
> > > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > -				      DP_SINK_DEVICE_AUX_FRAME_S
> > > YNC_CAP,
> > > -				      &frame_sync_cap) != 1)
> > > -			frame_sync_cap = 0;
> > > -		dev_priv->psr.aux_frame_sync = frame_sync_cap &
> > > DP_AUX_FRAME_SYNC_CAP;
> > > -		/* PSR2 needs frame sync as well */
> > > -		dev_priv->psr.psr2_support = dev_priv-
> > > >psr.aux_frame_sync;
> > > +		dev_priv->psr.psr2_support = true;
> > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > >  			      dev_priv->psr.psr2_support ?
> > > "supported" : "not supported");
> > >  
> > > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct
> > > intel_dp *intel_dp)
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  
> > >  
> > > -	/* Enable AUX frame sync at sink */
> > > -	if (dev_priv->psr.aux_frame_sync)
> > > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CO
> > > NF,
> > > -				DP_AUX_FRAME_SYNC_ENABLE);
> > >  	/* Enable ALPM at sink for psr2 */
> > >  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
> > >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  		i915_reg_t psr_status;
> > >  		u32 psr_status_mask;
> > >  
> > > -		if (dev_priv->psr.aux_frame_sync)
> > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > -					DP_SINK_DEVICE_AUX_FRAME
> > > _SYNC_CONF,
> > > -					0);
> > > -
> > >  		if (dev_priv->psr.psr2_support) {
> > >  			psr_status = EDP_PSR2_STATUS;
> > >  			psr_status_mask =
> > > EDP_PSR2_STATUS_STATE_MASK;
> > > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct
> > > drm_i915_private *dev_priv)
> > >  		return;
> > >  
> > >  	if (HAS_DDI(dev_priv)) {
> > > -		if (dev_priv->psr.aux_frame_sync)
> > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > -					DP_SINK_DEVICE_AUX_FRAME
> > > _SYNC_CONF,
> > > -					0);
> > >  		if (dev_priv->psr.psr2_support) {
> > >  			val = I915_READ(EDP_PSR2_CTL);
> > >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Pandiyan, Dhinakaran]

On Fri, 2018-03-23 at 23:49 +0000, Souza, Jose wrote:
> On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote:
> > On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> > > On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza
> > > wrote:
> > > > Without GTC enabled hardware is sending dummy aux frame sync
> > > > value
> > 
> > Curious, is this something you found by testing?
> 
> There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE
> register in sink, I'm reading this as valid but the value of aux frame
> in sink is always 0, the same as GTC_LIVE register in source side by
> this I guess source is sending 0 in each aux frame sync.
> 
> > 
> > > > that is not useful to sink do selective update, that is why it
> > > > also
> > > > require that sink supports and requires the y-coordinate.
> > > > 
> > > > So removing everything related to aux frame sync, if GTC is
> > > > enabled
> > > > we can bring this back.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > 
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > 
> > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > (but I would like to give a time for Vathsala to comment on this)
> > > 
> > 
> > Reading the spec again,
> > 
> > No aux frame sync implies that the driver does not support selective
> > update, irrespective of whether we *support y-coordinate or not*.
> > 
> > Section 7.1 eDP v1.4b
> > "AUX_FRAME_SYNC was added to eDP v1.4 for streamlining the RFB
> > management of a Sink device during PSR2 operation. AUX_FRAME_SYNC is
> > required a for PSR2 when using the Selective Update feature (which is
> > any time that the SU_VALID bit p is set to 1 in the PSR2 VSC SDP).
> > Neither GTC nor AUX_FRAME_SYNC is required for PSR2 when using only
> > the
> > Single-frame Update feature (which means that the SU_VALID bit is
> > always
> > cleared to 0 in the PSR2 VSC SDP)."
> > 
> > Sending y-coordinate only allows for a lower precision aux frame sync
> > functionality, doesn't invalidate the need for aux frame sync.
> > 
> > "When the Source device includes the optional Y-coordinate in the SDP
> > for PSR2 Operation, as described in Table 6-12, the Sink device can
> > implement a lower-precision GTC Slave  function, as described in
> > Table
> > 7-1."
> > 
> > Aside from the still slightly misleading commit message, this in my
> > opinion is a good step. Let's make it obvious and clear that only
> > full
> > frame updates are currently supported. We can then start working on
> > what's required for selective update and do it properly.
> > 
> > 
> > I'd like you to include relevant portions of the above text in the
> > commit message if you agree with the interpretation.
> > 
> > -DK
> 
> I agree with your interpretation of the spec but by the previous
> attempts of enabling of PSR/PSR2 and also by my tests the selective
> update with y-coordinate works at least in the pannels that we have
> access to.

I'll take your word on that. How are you checking selective update is
indeed working?

Vathsala/Rodrigo

Any idea why selective update seems to be working without aux frame
sync?

> And do not makes sense enable any PSR2 if we are going to do full frame
> updates, that is PSR1.

Yeah, but we do want to make sure selective update is working without
aux frame sync.

As the driver doesn't implement aux frame sync, there's not much meaning
in requiring the sink to support it. So, this patch is
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Please rewrite the commit message stating what's in the spec and include
the contradictory observation you made in the tests. Thanks for all the
work on this patch.

-DK

> 
> > 
> > 
> > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > > >  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
> > > >  2 files changed, 1 insertion(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index c9c3b2ba6a86..7fe00509e51a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -602,7 +602,6 @@ struct i915_psr {
> > > >  	struct delayed_work work;
> > > >  	unsigned busy_frontbuffer_bits;
> > > >  	bool psr2_support;
> > > > -	bool aux_frame_sync;
> > > >  	bool link_standby;
> > > >  	bool y_cord_support;
> > > >  	bool colorimetry_support;
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index b8e083e10029..d46320a451d9 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > > >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > > -		uint8_t frame_sync_cap;
> > > >  
> > > >  		dev_priv->psr.sink_support = true;
> > > > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > -				      DP_SINK_DEVICE_AUX_FRAME_S
> > > > YNC_CAP,
> > > > -				      &frame_sync_cap) != 1)
> > > > -			frame_sync_cap = 0;
> > > > -		dev_priv->psr.aux_frame_sync = frame_sync_cap &
> > > > DP_AUX_FRAME_SYNC_CAP;
> > > > -		/* PSR2 needs frame sync as well */
> > > > -		dev_priv->psr.psr2_support = dev_priv-
> > > > >psr.aux_frame_sync;
> > > > +		dev_priv->psr.psr2_support = true;
> > > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > >  			      dev_priv->psr.psr2_support ?
> > > > "supported" : "not supported");
> > > >  
> > > > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct
> > > > intel_dp *intel_dp)
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  
> > > >  
^ Extra line

> > > > -	/* Enable AUX frame sync at sink */
> > > > -	if (dev_priv->psr.aux_frame_sync)
> > > > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CO
> > > > NF,
> > > > -				DP_AUX_FRAME_SYNC_ENABLE);
> > > >  	/* Enable ALPM at sink for psr2 */
> > > >  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
> > > >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  		i915_reg_t psr_status;
> > > >  		u32 psr_status_mask;
> > > >  
> > > > -		if (dev_priv->psr.aux_frame_sync)
> > > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > -					DP_SINK_DEVICE_AUX_FRAME
> > > > _SYNC_CONF,
> > > > -					0);
> > > > -
> > > >  		if (dev_priv->psr.psr2_support) {
> > > >  			psr_status = EDP_PSR2_STATUS;
> > > >  			psr_status_mask =
> > > > EDP_PSR2_STATUS_STATE_MASK;
> > > > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct
> > > > drm_i915_private *dev_priv)
> > > >  		return;
> > > >  
> > > >  	if (HAS_DDI(dev_priv)) {
> > > > -		if (dev_priv->psr.aux_frame_sync)
> > > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > -					DP_SINK_DEVICE_AUX_FRAME
> > > > _SYNC_CONF,
> > > > -					0);
> > > >  		if (dev_priv->psr.psr2_support) {
> > > >  			val = I915_READ(EDP_PSR2_CTL);
> > > >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > > > -- 
> > > > 2.16.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

[Souza, Jose]

On Fri, 2018-03-23 at 19:16 -0700, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-03-23 at 23:49 +0000, Souza, Jose wrote:
> > On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> > > > On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza
> > > > wrote:
> > > > > Without GTC enabled hardware is sending dummy aux frame sync
> > > > > value
> > > 
> > > Curious, is this something you found by testing?
> > 
> > There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE
> > register in sink, I'm reading this as valid but the value of aux
> > frame
> > in sink is always 0, the same as GTC_LIVE register in source side
> > by
> > this I guess source is sending 0 in each aux frame sync.
> > 
> > > 
> > > > > that is not useful to sink do selective update, that is why
> > > > > it
> > > > > also
> > > > > require that sink supports and requires the y-coordinate.
> > > > > 
> > > > > So removing everything related to aux frame sync, if GTC is
> > > > > enabled
> > > > > we can bring this back.
> > > > > 
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > 
> > > > Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > 
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > 
> > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > (but I would like to give a time for Vathsala to comment on
> > > > this)
> > > > 
> > > 
> > > Reading the spec again,
> > > 
> > > No aux frame sync implies that the driver does not support
> > > selective
> > > update, irrespective of whether we *support y-coordinate or not*.
> > > 
> > > Section 7.1 eDP v1.4b
> > > "AUX_FRAME_SYNC was added to eDP v1.4 for streamlining the RFB
> > > management of a Sink device during PSR2 operation. AUX_FRAME_SYNC
> > > is
> > > required a for PSR2 when using the Selective Update feature
> > > (which is
> > > any time that the SU_VALID bit p is set to 1 in the PSR2 VSC
> > > SDP).
> > > Neither GTC nor AUX_FRAME_SYNC is required for PSR2 when using
> > > only
> > > the
> > > Single-frame Update feature (which means that the SU_VALID bit is
> > > always
> > > cleared to 0 in the PSR2 VSC SDP)."
> > > 
> > > Sending y-coordinate only allows for a lower precision aux frame
> > > sync
> > > functionality, doesn't invalidate the need for aux frame sync.
> > > 
> > > "When the Source device includes the optional Y-coordinate in the
> > > SDP
> > > for PSR2 Operation, as described in Table 6-12, the Sink device
> > > can
> > > implement a lower-precision GTC Slave  function, as described in
> > > Table
> > > 7-1."
> > > 
> > > Aside from the still slightly misleading commit message, this in
> > > my
> > > opinion is a good step. Let's make it obvious and clear that only
> > > full
> > > frame updates are currently supported. We can then start working
> > > on
> > > what's required for selective update and do it properly.
> > > 
> > > 
> > > I'd like you to include relevant portions of the above text in
> > > the
> > > commit message if you agree with the interpretation.
> > > 
> > > -DK
> > 
> > I agree with your interpretation of the spec but by the previous
> > attempts of enabling of PSR/PSR2 and also by my tests the selective
> > update with y-coordinate works at least in the pannels that we have
> > access to.
> 
> I'll take your word on that. How are you checking selective update is
> indeed working?

Doing a lot of reads to i915_edp_psr_status debugfs with all the
patches in this series, I can see sink status as "active receiving
selective update" and on the SDP I can see that the Y-Coordinate is
valid and the image on sink is alright. 

> 
> Vathsala/Rodrigo
> 
> Any idea why selective update seems to be working without aux frame
> sync?
> 
> > And do not makes sense enable any PSR2 if we are going to do full
> > frame
> > updates, that is PSR1.
> 
> Yeah, but we do want to make sure selective update is working without
> aux frame sync.
> 
> As the driver doesn't implement aux frame sync, there's not much
> meaning
> in requiring the sink to support it. So, this patch is
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Please rewrite the commit message stating what's in the spec and
> include
> the contradictory observation you made in the tests. Thanks for all
> the
> work on this patch.

Sure.

Changing it to:


eDP spec states that aux frame is required to do PSR2 selective
update but i915 don't fully implement it. It sends the aux frame
sync messages but the value is always zero as the GTC is not enabled
in driver.

Through tests was findout that pannels can do selective update when
the y-coordinate is also included in SDP, that is why it is required
to run PSR2 in i915.

A dummy value is not useful at all to sink, so removing everything
related to aux frame sync, if GTC is enabled we can bring this back.


> 
> -DK
> 
> > 
> > > 
> > > 
> > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > > > >  drivers/gpu/drm/i915/intel_psr.c | 23 +---------------------
> > > > > -
> > > > >  2 files changed, 1 insertion(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index c9c3b2ba6a86..7fe00509e51a 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -602,7 +602,6 @@ struct i915_psr {
> > > > >  	struct delayed_work work;
> > > > >  	unsigned busy_frontbuffer_bits;
> > > > >  	bool psr2_support;
> > > > > -	bool aux_frame_sync;
> > > > >  	bool link_standby;
> > > > >  	bool y_cord_support;
> > > > >  	bool colorimetry_support;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index b8e083e10029..d46320a451d9 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -137,16 +137,9 @@ void intel_psr_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > >  
> > > > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > > > >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED))
> > > > > {
> > > > > -		uint8_t frame_sync_cap;
> > > > >  
> > > > >  		dev_priv->psr.sink_support = true;
> > > > > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > > -				      DP_SINK_DEVICE_AUX_FRA
> > > > > ME_S
> > > > > YNC_CAP,
> > > > > -				      &frame_sync_cap) != 1)
> > > > > -			frame_sync_cap = 0;
> > > > > -		dev_priv->psr.aux_frame_sync =
> > > > > frame_sync_cap &
> > > > > DP_AUX_FRAME_SYNC_CAP;
> > > > > -		/* PSR2 needs frame sync as well */
> > > > > -		dev_priv->psr.psr2_support = dev_priv-
> > > > > > psr.aux_frame_sync;
> > > > > 
> > > > > +		dev_priv->psr.psr2_support = true;
> > > > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > > >  			      dev_priv->psr.psr2_support ?
> > > > > "supported" : "not supported");
> > > > >  
> > > > > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct
> > > > > intel_dp *intel_dp)
> > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > >  
> > > > >  
> 
> ^ Extra line

Nice catch.

> 
> > > > > -	/* Enable AUX frame sync at sink */
> > > > > -	if (dev_priv->psr.aux_frame_sync)
> > > > > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > -				DP_SINK_DEVICE_AUX_FRAME_SYN
> > > > > C_CO
> > > > > NF,
> > > > > -				DP_AUX_FRAME_SYNC_ENABLE);
> > > > >  	/* Enable ALPM at sink for psr2 */
> > > > >  	if (dev_priv->psr.psr2_support && dev_priv-
> > > > > >psr.alpm)
> > > > >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct
> > > > > intel_dp
> > > > > *intel_dp,
> > > > >  		i915_reg_t psr_status;
> > > > >  		u32 psr_status_mask;
> > > > >  
> > > > > -		if (dev_priv->psr.aux_frame_sync)
> > > > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > -					DP_SINK_DEVICE_AUX_F
> > > > > RAME
> > > > > _SYNC_CONF,
> > > > > -					0);
> > > > > -
> > > > >  		if (dev_priv->psr.psr2_support) {
> > > > >  			psr_status = EDP_PSR2_STATUS;
> > > > >  			psr_status_mask =
> > > > > EDP_PSR2_STATUS_STATE_MASK;
> > > > > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  		return;
> > > > >  
> > > > >  	if (HAS_DDI(dev_priv)) {
> > > > > -		if (dev_priv->psr.aux_frame_sync)
> > > > > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > > > > -					DP_SINK_DEVICE_AUX_F
> > > > > RAME
> > > > > _SYNC_CONF,
> > > > > -					0);
> > > > >  		if (dev_priv->psr.psr2_support) {
> > > > >  			val = I915_READ(EDP_PSR2_CTL);
> > > > >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > > > > -- 
> > > > > 2.16.2
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[José Roberto de Souza]

Move to only one place the sink requirements that the actual driver
needs to enable PSR2.

Also intel_psr2_config_valid() is called every time the crtc config
is computed, wasting some time every time it was checking for
Y coordinate requirement.

This allow us to nuke y_cord_support and some of VSC setup code that
was handling a scenario that would never happen(PSR2 without Y
coordinate).

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>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------------------
 2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fe00509e51a..cce32686fd75 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -603,7 +603,6 @@ struct i915_psr {
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool link_standby;
-	bool y_cord_support;
 	bool colorimetry_support;
 	bool alpm;
 	bool has_hw_tracking;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d46320a451d9..23f38ab10636 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
 	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
 }
 
-static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
+static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)
 {
 	uint8_t psr_caps = 0;
 
@@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
 			 sizeof(intel_dp->psr_dpcd));
 
-	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
+	if (intel_dp->psr_dpcd[0]) {
 		dev_priv->psr.sink_support = true;
 		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
 	}
 
 	if (INTEL_GEN(dev_priv) >= 9 &&
-	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
-
-		dev_priv->psr.sink_support = true;
-		dev_priv->psr.psr2_support = true;
+	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
+		/*
+		 * All panels that supports PSR version 03h (PSR2 +
+		 * Y-coordinate) can handle Y-coordinates in VSC but we are
+		 * only sure that it is going to be used when required by the
+		 * panel. This way panel is capable to do selective update
+		 * without a aux frame sync.
+		 *
+		 * To support PSR version 02h and PSR version 03h without
+		 * Y-coordinate requirement panels we would need to enable
+		 * GTC first.
+		 */
+		dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp);
 		DRM_DEBUG_KMS("PSR2 %s on sink",
 			      dev_priv->psr.psr2_support ? "supported" : "not supported");
 
 		if (dev_priv->psr.psr2_support) {
-			dev_priv->psr.y_cord_support =
-				intel_dp_get_y_cord_status(intel_dp);
 			dev_priv->psr.colorimetry_support =
 				intel_dp_get_colorimetry_status(intel_dp);
 			dev_priv->psr.alpm =
@@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
 		memset(&psr_vsc, 0, sizeof(psr_vsc));
 		psr_vsc.sdp_header.HB0 = 0;
 		psr_vsc.sdp_header.HB1 = 0x7;
-		if (dev_priv->psr.colorimetry_support &&
-		    dev_priv->psr.y_cord_support) {
+		if (dev_priv->psr.colorimetry_support) {
 			psr_vsc.sdp_header.HB2 = 0x5;
 			psr_vsc.sdp_header.HB3 = 0x13;
-		} else if (dev_priv->psr.y_cord_support) {
+		} else {
 			psr_vsc.sdp_header.HB2 = 0x4;
 			psr_vsc.sdp_header.HB3 = 0xe;
-		} else {
-			psr_vsc.sdp_header.HB2 = 0x3;
-			psr_vsc.sdp_header.HB3 = 0xc;
 		}
 	} else {
 		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	/*
-	 * FIXME:enable psr2 only for y-cordinate psr2 panels
-	 * After gtc implementation , remove this restriction.
-	 */
-	if (!dev_priv->psr.y_cord_support) {
-		DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n");
-		return false;
-	}
-
 	return true;
 }
 
@@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	u32 chicken;
 
 	psr_aux_io_power_get(intel_dp);
 
@@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 		hsw_psr_setup_aux(intel_dp);
 
 	if (dev_priv->psr.psr2_support) {
-		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
-		if (dev_priv->psr.y_cord_support)
-			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
+		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
+			      | PSR2_ADD_VERTICAL_LINE_COUNT;
 		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
 
 		I915_WRITE(EDP_PSR_DEBUG,
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:40PM -0700, José Roberto de Souza wrote:
> Move to only one place the sink requirements that the actual driver
> needs to enable PSR2.
> 
> Also intel_psr2_config_valid() is called every time the crtc config
> is computed, wasting some time every time it was checking for
> Y coordinate requirement.
> 
> This allow us to nuke y_cord_support and some of VSC setup code that
> was handling a scenario that would never happen(PSR2 without Y
> coordinate).
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------------------
>  2 files changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7fe00509e51a..cce32686fd75 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -603,7 +603,6 @@ struct i915_psr {
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool link_standby;
> -	bool y_cord_support;
>  	bool colorimetry_support;
>  	bool alpm;
>  	bool has_hw_tracking;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d46320a451d9..23f38ab10636 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
>  	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
>  }
>  
> -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)

nip: I don't agree that required is a good name for this function call
so maybe we should just leave status and minimize the change.

>  {
>  	uint8_t psr_caps = 0;
>  
> @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>  			 sizeof(intel_dp->psr_dpcd));
>  
> -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> +	if (intel_dp->psr_dpcd[0]) {

hm? why?

>  		dev_priv->psr.sink_support = true;
>  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
> -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> -
> -		dev_priv->psr.sink_support = true;
> -		dev_priv->psr.psr2_support = true;
> +	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> +		/*
> +		 * All panels that supports PSR version 03h (PSR2 +
> +		 * Y-coordinate) can handle Y-coordinates in VSC but we are
> +		 * only sure that it is going to be used when required by the
> +		 * panel. This way panel is capable to do selective update
> +		 * without a aux frame sync.
> +		 *
> +		 * To support PSR version 02h and PSR version 03h without
> +		 * Y-coordinate requirement panels we would need to enable
> +		 * GTC first.
> +		 */
> +		dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp);

oh ok ok... now I saw why you changed to "required"

But my question now is. Do we really need to check this bit?

I believe with your change to check version 03h we don't need to check
this requirement anymore.
This looks like in the past we used that to workaround a way to identify
if the panel was really y-coord... but now with 03h version we don't need
to check if panel is requiring the y-coordination from the source, because
we know what source is capable of.

>  		DRM_DEBUG_KMS("PSR2 %s on sink",
>  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>  
>  		if (dev_priv->psr.psr2_support) {
> -			dev_priv->psr.y_cord_support =
> -				intel_dp_get_y_cord_status(intel_dp);
>  			dev_priv->psr.colorimetry_support =
>  				intel_dp_get_colorimetry_status(intel_dp);
>  			dev_priv->psr.alpm =
> @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
>  		memset(&psr_vsc, 0, sizeof(psr_vsc));
>  		psr_vsc.sdp_header.HB0 = 0;
>  		psr_vsc.sdp_header.HB1 = 0x7;
> -		if (dev_priv->psr.colorimetry_support &&
> -		    dev_priv->psr.y_cord_support) {
> +		if (dev_priv->psr.colorimetry_support) {
>  			psr_vsc.sdp_header.HB2 = 0x5;
>  			psr_vsc.sdp_header.HB3 = 0x13;
> -		} else if (dev_priv->psr.y_cord_support) {
> +		} else {
>  			psr_vsc.sdp_header.HB2 = 0x4;
>  			psr_vsc.sdp_header.HB3 = 0xe;
> -		} else {
> -			psr_vsc.sdp_header.HB2 = 0x3;
> -			psr_vsc.sdp_header.HB3 = 0xc;
>  		}
>  	} else {
>  		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	/*
> -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> -	 * After gtc implementation , remove this restriction.
> -	 */
> -	if (!dev_priv->psr.y_cord_support) {
> -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n");
> -		return false;
> -	}
> -
>  	return true;
>  }
>  
> @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	u32 chicken;
>  
>  	psr_aux_io_power_get(intel_dp);
>  
> @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		hsw_psr_setup_aux(intel_dp);
>  
>  	if (dev_priv->psr.psr2_support) {
> -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> -		if (dev_priv->psr.y_cord_support)
> -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
>  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
>  
>  		I915_WRITE(EDP_PSR_DEBUG,
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Souza, Jose]

On Thu, 2018-03-22 at 16:09 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:40PM -0700, José Roberto de Souza
> wrote:
> > Move to only one place the sink requirements that the actual driver
> > needs to enable PSR2.
> > 
> > Also intel_psr2_config_valid() is called every time the crtc config
> > is computed, wasting some time every time it was checking for
> > Y coordinate requirement.
> > 
> > This allow us to nuke y_cord_support and some of VSC setup code
> > that
> > was handling a scenario that would never happen(PSR2 without Y
> > coordinate).
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------
> > ------------
> >  2 files changed, 19 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 7fe00509e51a..cce32686fd75 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -603,7 +603,6 @@ struct i915_psr {
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> >  	bool link_standby;
> > -	bool y_cord_support;
> >  	bool colorimetry_support;
> >  	bool alpm;
> >  	bool has_hw_tracking;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d46320a451d9..23f38ab10636 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > *intel_dp)
> >  	intel_display_power_put(dev_priv,
> > psr_aux_domain(intel_dp));
> >  }
> >  
> > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > *intel_dp)
> 
> nip: I don't agree that required is a good name for this function
> call
> so maybe we should just leave status and minimize the change.

This is the name in the eDP spec: Y-coordinate Requirement of Sink
Device on PSR2 Selective Update

> 
> >  {
> >  	uint8_t psr_caps = 0;
> >  
> > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > >psr_dpcd,
> >  			 sizeof(intel_dp->psr_dpcd));
> >  
> > -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > +	if (intel_dp->psr_dpcd[0]) {
> 
> hm? why?

A pannel that is PSR version 2, will not have any PSR enabled as
'intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED' and 'intel_dp-
>psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED' will be false.

> 
> >  		dev_priv->psr.sink_support = true;
> >  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > -
> > -		dev_priv->psr.sink_support = true;
> > -		dev_priv->psr.psr2_support = true;
> > +	    (intel_dp->psr_dpcd[0] ==
> > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > +		/*
> > +		 * All panels that supports PSR version 03h (PSR2
> > +
> > +		 * Y-coordinate) can handle Y-coordinates in VSC
> > but we are
> > +		 * only sure that it is going to be used when
> > required by the
> > +		 * panel. This way panel is capable to do
> > selective update
> > +		 * without a aux frame sync.
> > +		 *
> > +		 * To support PSR version 02h and PSR version 03h
> > without
> > +		 * Y-coordinate requirement panels we would need
> > to enable
> > +		 * GTC first.
> > +		 */
> > +		dev_priv->psr.psr2_support =
> > intel_dp_get_y_coord_required(intel_dp);
> 
> oh ok ok... now I saw why you changed to "required"
> 
> But my question now is. Do we really need to check this bit?
> 
> I believe with your change to check version 03h we don't need to
> check
> this requirement anymore.
> This looks like in the past we used that to workaround a way to
> identify
> if the panel was really y-coord... but now with 03h version we don't
> need
> to check if panel is requiring the y-coordination from the source,
> because
> we know what source is capable of.

We know that is capable of but we don't know if is going to be used by
the pannel, when pannels require the y-coordinate we are 100% sure that
it is going to use.

> 
> >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> >  			      dev_priv->psr.psr2_support ?
> > "supported" : "not supported");
> >  
> >  		if (dev_priv->psr.psr2_support) {
> > -			dev_priv->psr.y_cord_support =
> > -				intel_dp_get_y_cord_status(intel_d
> > p);
> >  			dev_priv->psr.colorimetry_support =
> >  				intel_dp_get_colorimetry_status(in
> > tel_dp);
> >  			dev_priv->psr.alpm =
> > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > *intel_dp,
> >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> >  		psr_vsc.sdp_header.HB0 = 0;
> >  		psr_vsc.sdp_header.HB1 = 0x7;
> > -		if (dev_priv->psr.colorimetry_support &&
> > -		    dev_priv->psr.y_cord_support) {
> > +		if (dev_priv->psr.colorimetry_support) {
> >  			psr_vsc.sdp_header.HB2 = 0x5;
> >  			psr_vsc.sdp_header.HB3 = 0x13;
> > -		} else if (dev_priv->psr.y_cord_support) {
> > +		} else {
> >  			psr_vsc.sdp_header.HB2 = 0x4;
> >  			psr_vsc.sdp_header.HB3 = 0xe;
> > -		} else {
> > -			psr_vsc.sdp_header.HB2 = 0x3;
> > -			psr_vsc.sdp_header.HB3 = 0xc;
> >  		}
> >  	} else {
> >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > 3.10 */
> > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > -	/*
> > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > -	 * After gtc implementation , remove this restriction.
> > -	 */
> > -	if (!dev_priv->psr.y_cord_support) {
> > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > support Y coordinate\n");
> > -		return false;
> > -	}
> > -
> >  	return true;
> >  }
> >  
> > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> > -	u32 chicken;
> >  
> >  	psr_aux_io_power_get(intel_dp);
> >  
> > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  		hsw_psr_setup_aux(intel_dp);
> >  
> >  	if (dev_priv->psr.psr2_support) {
> > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > -		if (dev_priv->psr.y_cord_support)
> > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > chicken);
> >  
> >  		I915_WRITE(EDP_PSR_DEBUG,
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Pandiyan, Dhinakaran]

On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote:
> Move to only one place the sink requirements that the actual driver
> needs to enable PSR2.
> 
> Also intel_psr2_config_valid() is called every time the crtc config
> is computed, wasting some time every time it was checking for
> Y coordinate requirement.
> 
> This allow us to nuke y_cord_support and some of VSC setup code that
> was handling a scenario that would never happen(PSR2 without Y
> coordinate).
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------------------
>  2 files changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7fe00509e51a..cce32686fd75 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -603,7 +603,6 @@ struct i915_psr {
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool link_standby;
> -	bool y_cord_support;
>  	bool colorimetry_support;
>  	bool alpm;
>  	bool has_hw_tracking;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d46320a451d9..23f38ab10636 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
>  	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
>  }
>  
> -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> +static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)
>  {
>  	uint8_t psr_caps = 0;
>  
> @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>  			 sizeof(intel_dp->psr_dpcd));
>  
> -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> +	if (intel_dp->psr_dpcd[0]) {
>  		dev_priv->psr.sink_support = true;
>  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
> -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> -
> -		dev_priv->psr.sink_support = true;
> -		dev_priv->psr.psr2_support = true;
> +	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> +		/*
> +		 * All panels that supports PSR version 03h (PSR2 +
> +		 * Y-coordinate) can handle Y-coordinates in VSC but we are
> +		 * only sure that it is going to be used when required by the
> +		 * panel. This way panel is capable to do selective update
> +		 * without a aux frame sync.

Can you cite this from the spec please? just the "panel is capable to do
selective update without a aux frame sync" part. Or you could just
remove that line so that we can get this patch reviewed and merged.


> +		 *
> +		 * To support PSR version 02h and PSR version 03h without
> +		 * Y-coordinate requirement panels we would need to enable
> +		 * GTC first.
> +		 */
> +		dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp);
>  		DRM_DEBUG_KMS("PSR2 %s on sink",
>  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>  
>  		if (dev_priv->psr.psr2_support) {
> -			dev_priv->psr.y_cord_support =
> -				intel_dp_get_y_cord_status(intel_dp);
>  			dev_priv->psr.colorimetry_support =
>  				intel_dp_get_colorimetry_status(intel_dp);
>  			dev_priv->psr.alpm =
> @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
>  		memset(&psr_vsc, 0, sizeof(psr_vsc));
>  		psr_vsc.sdp_header.HB0 = 0;
>  		psr_vsc.sdp_header.HB1 = 0x7;
> -		if (dev_priv->psr.colorimetry_support &&
> -		    dev_priv->psr.y_cord_support) {
> +		if (dev_priv->psr.colorimetry_support) {
>  			psr_vsc.sdp_header.HB2 = 0x5;
>  			psr_vsc.sdp_header.HB3 = 0x13;
> -		} else if (dev_priv->psr.y_cord_support) {
> +		} else {
>  			psr_vsc.sdp_header.HB2 = 0x4;
>  			psr_vsc.sdp_header.HB3 = 0xe;
> -		} else {
> -			psr_vsc.sdp_header.HB2 = 0x3;
> -			psr_vsc.sdp_header.HB3 = 0xc;
>  		}
>  	} else {
>  		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	/*
> -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> -	 * After gtc implementation , remove this restriction.
> -	 */
> -	if (!dev_priv->psr.y_cord_support) {
> -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not support Y coordinate\n");
> -		return false;
> -	}
> -
>  	return true;
>  }
>  
> @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	u32 chicken;
>  
>  	psr_aux_io_power_get(intel_dp);
>  
> @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  		hsw_psr_setup_aux(intel_dp);
>  
>  	if (dev_priv->psr.psr2_support) {
> -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> -		if (dev_priv->psr.y_cord_support)
> -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
>  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
>  
>  		I915_WRITE(EDP_PSR_DEBUG,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Souza, Jose]

On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote:
> > Move to only one place the sink requirements that the actual driver
> > needs to enable PSR2.
> > 
> > Also intel_psr2_config_valid() is called every time the crtc config
> > is computed, wasting some time every time it was checking for
> > Y coordinate requirement.
> > 
> > This allow us to nuke y_cord_support and some of VSC setup code
> > that
> > was handling a scenario that would never happen(PSR2 without Y
> > coordinate).
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------
> > ------------
> >  2 files changed, 19 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 7fe00509e51a..cce32686fd75 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -603,7 +603,6 @@ struct i915_psr {
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> >  	bool link_standby;
> > -	bool y_cord_support;
> >  	bool colorimetry_support;
> >  	bool alpm;
> >  	bool has_hw_tracking;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d46320a451d9..23f38ab10636 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > *intel_dp)
> >  	intel_display_power_put(dev_priv,
> > psr_aux_domain(intel_dp));
> >  }
> >  
> > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > *intel_dp)
> >  {
> >  	uint8_t psr_caps = 0;
> >  
> > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > >psr_dpcd,
> >  			 sizeof(intel_dp->psr_dpcd));
> >  
> > -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > +	if (intel_dp->psr_dpcd[0]) {
> >  		dev_priv->psr.sink_support = true;
> >  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > -
> > -		dev_priv->psr.sink_support = true;
> > -		dev_priv->psr.psr2_support = true;
> > +	    (intel_dp->psr_dpcd[0] ==
> > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > +		/*
> > +		 * All panels that supports PSR version 03h (PSR2
> > +
> > +		 * Y-coordinate) can handle Y-coordinates in VSC
> > but we are
> > +		 * only sure that it is going to be used when
> > required by the
> > +		 * panel. This way panel is capable to do
> > selective update
> > +		 * without a aux frame sync.
> 
> Can you cite this from the spec please? just the "panel is capable to
> do
> selective update without a aux frame sync" part. Or you could just
> remove that line so that we can get this patch reviewed and merged.

There is no such statement in spec like this, the closest that it have
is:
"When the Source device includes the optional Y-coordinate in the SDP
for PSR2 Operation, as described in Table 6-12, the Sink device can
implement a lower-precision GTC Slave  function, as described in Table
7-1."

But we know that this works by all the previous tests when enabling
PSR2 and I also tested it.

> 
> 
> > +		 *
> > +		 * To support PSR version 02h and PSR version 03h
> > without
> > +		 * Y-coordinate requirement panels we would need
> > to enable
> > +		 * GTC first.
> > +		 */
> > +		dev_priv->psr.psr2_support =
> > intel_dp_get_y_coord_required(intel_dp);
> >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> >  			      dev_priv->psr.psr2_support ?
> > "supported" : "not supported");
> >  
> >  		if (dev_priv->psr.psr2_support) {
> > -			dev_priv->psr.y_cord_support =
> > -				intel_dp_get_y_cord_status(intel_d
> > p);
> >  			dev_priv->psr.colorimetry_support =
> >  				intel_dp_get_colorimetry_status(in
> > tel_dp);
> >  			dev_priv->psr.alpm =
> > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > *intel_dp,
> >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> >  		psr_vsc.sdp_header.HB0 = 0;
> >  		psr_vsc.sdp_header.HB1 = 0x7;
> > -		if (dev_priv->psr.colorimetry_support &&
> > -		    dev_priv->psr.y_cord_support) {
> > +		if (dev_priv->psr.colorimetry_support) {
> >  			psr_vsc.sdp_header.HB2 = 0x5;
> >  			psr_vsc.sdp_header.HB3 = 0x13;
> > -		} else if (dev_priv->psr.y_cord_support) {
> > +		} else {
> >  			psr_vsc.sdp_header.HB2 = 0x4;
> >  			psr_vsc.sdp_header.HB3 = 0xe;
> > -		} else {
> > -			psr_vsc.sdp_header.HB2 = 0x3;
> > -			psr_vsc.sdp_header.HB3 = 0xc;
> >  		}
> >  	} else {
> >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > 3.10 */
> > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > -	/*
> > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > -	 * After gtc implementation , remove this restriction.
> > -	 */
> > -	if (!dev_priv->psr.y_cord_support) {
> > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > support Y coordinate\n");
> > -		return false;
> > -	}
> > -
> >  	return true;
> >  }
> >  
> > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> > -	u32 chicken;
> >  
> >  	psr_aux_io_power_get(intel_dp);
> >  
> > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  		hsw_psr_setup_aux(intel_dp);
> >  
> >  	if (dev_priv->psr.psr2_support) {
> > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > -		if (dev_priv->psr.y_cord_support)
> > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > chicken);
> >  
> >  		I915_WRITE(EDP_PSR_DEBUG,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Pandiyan, Dhinakaran]

On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote:
> On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote:
> > > Move to only one place the sink requirements that the actual driver
> > > needs to enable PSR2.
> > > 
> > > Also intel_psr2_config_valid() is called every time the crtc config
> > > is computed, wasting some time every time it was checking for
> > > Y coordinate requirement.
> > > 
> > > This allow us to nuke y_cord_support and some of VSC setup code
> > > that
> > > was handling a scenario that would never happen(PSR2 without Y
> > > coordinate).
> > > 
> > > 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>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > >  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------
> > > ------------
> > >  2 files changed, 19 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7fe00509e51a..cce32686fd75 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -603,7 +603,6 @@ struct i915_psr {
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool psr2_support;
> > >  	bool link_standby;
> > > -	bool y_cord_support;
> > >  	bool colorimetry_support;
> > >  	bool alpm;
> > >  	bool has_hw_tracking;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d46320a451d9..23f38ab10636 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > > *intel_dp)
> > >  	intel_display_power_put(dev_priv,
> > > psr_aux_domain(intel_dp));
> > >  }
> > >  
> > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > > *intel_dp)
> > >  {
> > >  	uint8_t psr_caps = 0;
> > >  
> > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > > >psr_dpcd,
> > >  			 sizeof(intel_dp->psr_dpcd));
> > >  
> > > -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > > +	if (intel_dp->psr_dpcd[0]) {
> > >  		dev_priv->psr.sink_support = true;
> > >  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > >  	}
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > > -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > -
> > > -		dev_priv->psr.sink_support = true;
> > > -		dev_priv->psr.psr2_support = true;
> > > +	    (intel_dp->psr_dpcd[0] ==
> > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > > +		/*
> > > +		 * All panels that supports PSR version 03h (PSR2
> > > +
> > > +		 * Y-coordinate) can handle Y-coordinates in VSC
> > > but we are
> > > +		 * only sure that it is going to be used when
> > > required by the
> > > +		 * panel. This way panel is capable to do
> > > selective update
> > > +		 * without a aux frame sync.
> > 
> > Can you cite this from the spec please? just the "panel is capable to
> > do
> > selective update without a aux frame sync" part. Or you could just
> > remove that line so that we can get this patch reviewed and merged.
> 
> There is no such statement in spec like this, the closest that it have
> is:
> "When the Source device includes the optional Y-coordinate in the SDP
> for PSR2 Operation, as described in Table 6-12, the Sink device can
> implement a lower-precision GTC Slave  function, as described in Table
> 7-1."
> 
> But we know that this works by all the previous tests when enabling
> PSR2 and I also tested it.

Please do update the comment to say so. Do you know if these tests for
PSR2 selective update are in IGT? Or were these manual tests?

Rodrigo/Vathsala, any idea how selective update was and can be tested?

The patch as such makes sense to me
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if you
update the comment.


> 
> > 
> > 
> > > +		 *
> > > +		 * To support PSR version 02h and PSR version 03h
> > > without
> > > +		 * Y-coordinate requirement panels we would need
> > > to enable
> > > +		 * GTC first.
> > > +		 */
> > > +		dev_priv->psr.psr2_support =
> > > intel_dp_get_y_coord_required(intel_dp);
> > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > >  			      dev_priv->psr.psr2_support ?
> > > "supported" : "not supported");
> > >  
> > >  		if (dev_priv->psr.psr2_support) {
> > > -			dev_priv->psr.y_cord_support =
> > > -				intel_dp_get_y_cord_status(intel_d
> > > p);
> > >  			dev_priv->psr.colorimetry_support =
> > >  				intel_dp_get_colorimetry_status(in
> > > tel_dp);
> > >  			dev_priv->psr.alpm =
> > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > > *intel_dp,
> > >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> > >  		psr_vsc.sdp_header.HB0 = 0;
> > >  		psr_vsc.sdp_header.HB1 = 0x7;
> > > -		if (dev_priv->psr.colorimetry_support &&
> > > -		    dev_priv->psr.y_cord_support) {
> > > +		if (dev_priv->psr.colorimetry_support) {
> > >  			psr_vsc.sdp_header.HB2 = 0x5;
> > >  			psr_vsc.sdp_header.HB3 = 0x13;
> > > -		} else if (dev_priv->psr.y_cord_support) {
> > > +		} else {
> > >  			psr_vsc.sdp_header.HB2 = 0x4;
> > >  			psr_vsc.sdp_header.HB3 = 0xe;
> > > -		} else {
> > > -			psr_vsc.sdp_header.HB2 = 0x3;
> > > -			psr_vsc.sdp_header.HB3 = 0xc;
> > >  		}
> > >  	} else {
> > >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > > 3.10 */
> > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > > -	 * After gtc implementation , remove this restriction.
> > > -	 */
> > > -	if (!dev_priv->psr.y_cord_support) {
> > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > > support Y coordinate\n");
> > > -		return false;
> > > -	}
> > > -
> > >  	return true;
> > >  }
> > >  
> > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  	struct drm_device *dev = dig_port->base.base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > > -	u32 chicken;
> > >  
> > >  	psr_aux_io_power_get(intel_dp);
> > >  
> > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  		hsw_psr_setup_aux(intel_dp);
> > >  
> > >  	if (dev_priv->psr.psr2_support) {
> > > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > -		if (dev_priv->psr.y_cord_support)
> > > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> > >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > > chicken);
> > >  
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Rodrigo Vivi]

On Fri, Mar 23, 2018 at 07:34:34PM -0700, Pandiyan, Dhinakaran wrote:
> 
> On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote:
> > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote:
> > > > Move to only one place the sink requirements that the actual driver
> > > > needs to enable PSR2.
> > > > 
> > > > Also intel_psr2_config_valid() is called every time the crtc config
> > > > is computed, wasting some time every time it was checking for
> > > > Y coordinate requirement.
> > > > 
> > > > This allow us to nuke y_cord_support and some of VSC setup code
> > > > that
> > > > was handling a scenario that would never happen(PSR2 without Y
> > > > coordinate).
> > > > 
> > > > 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>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > > >  drivers/gpu/drm/i915/intel_psr.c | 46 +++++++++++++++++-----------
> > > > ------------
> > > >  2 files changed, 19 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 7fe00509e51a..cce32686fd75 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -603,7 +603,6 @@ struct i915_psr {
> > > >  	unsigned busy_frontbuffer_bits;
> > > >  	bool psr2_support;
> > > >  	bool link_standby;
> > > > -	bool y_cord_support;
> > > >  	bool colorimetry_support;
> > > >  	bool alpm;
> > > >  	bool has_hw_tracking;
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d46320a451d9..23f38ab10636 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > > > *intel_dp)
> > > >  	intel_display_power_put(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > >  }
> > > >  
> > > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > > > *intel_dp)
> > > >  {
> > > >  	uint8_t psr_caps = 0;
> > > >  
> > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > > > >psr_dpcd,
> > > >  			 sizeof(intel_dp->psr_dpcd));
> > > >  
> > > > -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > > > +	if (intel_dp->psr_dpcd[0]) {
> > > >  		dev_priv->psr.sink_support = true;
> > > >  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > > >  	}
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > > > -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > > -
> > > > -		dev_priv->psr.sink_support = true;
> > > > -		dev_priv->psr.psr2_support = true;
> > > > +	    (intel_dp->psr_dpcd[0] ==
> > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > > > +		/*
> > > > +		 * All panels that supports PSR version 03h (PSR2
> > > > +
> > > > +		 * Y-coordinate) can handle Y-coordinates in VSC
> > > > but we are
> > > > +		 * only sure that it is going to be used when
> > > > required by the
> > > > +		 * panel. This way panel is capable to do
> > > > selective update
> > > > +		 * without a aux frame sync.
> > > 
> > > Can you cite this from the spec please? just the "panel is capable to
> > > do
> > > selective update without a aux frame sync" part. Or you could just
> > > remove that line so that we can get this patch reviewed and merged.
> > 
> > There is no such statement in spec like this, the closest that it have
> > is:
> > "When the Source device includes the optional Y-coordinate in the SDP
> > for PSR2 Operation, as described in Table 6-12, the Sink device can
> > implement a lower-precision GTC Slave  function, as described in Table
> > 7-1."
> > 
> > But we know that this works by all the previous tests when enabling
> > PSR2 and I also tested it.
> 
> Please do update the comment to say so. Do you know if these tests for
> PSR2 selective update are in IGT? Or were these manual tests?
> 
> Rodrigo/Vathsala, any idea how selective update was and can be tested?

nope :(

> 
> The patch as such makes sense to me
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if you
> update the comment.
> 
> 
> > 
> > > 
> > > 
> > > > +		 *
> > > > +		 * To support PSR version 02h and PSR version 03h
> > > > without
> > > > +		 * Y-coordinate requirement panels we would need
> > > > to enable
> > > > +		 * GTC first.
> > > > +		 */
> > > > +		dev_priv->psr.psr2_support =
> > > > intel_dp_get_y_coord_required(intel_dp);
> > > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > >  			      dev_priv->psr.psr2_support ?
> > > > "supported" : "not supported");
> > > >  
> > > >  		if (dev_priv->psr.psr2_support) {
> > > > -			dev_priv->psr.y_cord_support =
> > > > -				intel_dp_get_y_cord_status(intel_d
> > > > p);
> > > >  			dev_priv->psr.colorimetry_support =
> > > >  				intel_dp_get_colorimetry_status(in
> > > > tel_dp);
> > > >  			dev_priv->psr.alpm =
> > > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > > > *intel_dp,
> > > >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> > > >  		psr_vsc.sdp_header.HB0 = 0;
> > > >  		psr_vsc.sdp_header.HB1 = 0x7;
> > > > -		if (dev_priv->psr.colorimetry_support &&
> > > > -		    dev_priv->psr.y_cord_support) {
> > > > +		if (dev_priv->psr.colorimetry_support) {
> > > >  			psr_vsc.sdp_header.HB2 = 0x5;
> > > >  			psr_vsc.sdp_header.HB3 = 0x13;
> > > > -		} else if (dev_priv->psr.y_cord_support) {
> > > > +		} else {
> > > >  			psr_vsc.sdp_header.HB2 = 0x4;
> > > >  			psr_vsc.sdp_header.HB3 = 0xe;
> > > > -		} else {
> > > > -			psr_vsc.sdp_header.HB2 = 0x3;
> > > > -			psr_vsc.sdp_header.HB3 = 0xc;
> > > >  		}
> > > >  	} else {
> > > >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > > > 3.10 */
> > > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > > > -	 * After gtc implementation , remove this restriction.
> > > > -	 */
> > > > -	if (!dev_priv->psr.y_cord_support) {
> > > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > > > support Y coordinate\n");
> > > > -		return false;
> > > > -	}
> > > > -
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > >  	struct drm_device *dev = dig_port->base.base.dev;
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	enum transcoder cpu_transcoder = crtc_state-
> > > > >cpu_transcoder;
> > > > -	u32 chicken;
> > > >  
> > > >  	psr_aux_io_power_get(intel_dp);
> > > >  
> > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > >  		hsw_psr_setup_aux(intel_dp);
> > > >  
> > > >  	if (dev_priv->psr.psr2_support) {
> > > > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > -		if (dev_priv->psr.y_cord_support)
> > > > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > > > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > > > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> > > >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > > > chicken);
> > > >  
> > > >  		I915_WRITE(EDP_PSR_DEBUG,
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

[Nagaraju, Vathsala]

Selective update testing
	 play a video and check 0x60094 , (03, 06) will indicated that system is in su state.


-----Original Message-----
From: Vivi, Rodrigo 
Sent: Wednesday, March 28, 2018 3:07 AM
To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
Cc: Souza, Jose <jose.souza@intel.com>; intel-gfx@lists.freedesktop.org; Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
Subject: Re: [Intel-gfx] [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement

On Fri, Mar 23, 2018 at 07:34:34PM -0700, Pandiyan, Dhinakaran wrote:
> 
> On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote:
> > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > On Thu, 2018-03-22 at 14:48 -0700, José Roberto de Souza wrote:
> > > > Move to only one place the sink requirements that the actual 
> > > > driver needs to enable PSR2.
> > > > 
> > > > Also intel_psr2_config_valid() is called every time the crtc 
> > > > config is computed, wasting some time every time it was checking 
> > > > for Y coordinate requirement.
> > > > 
> > > > This allow us to nuke y_cord_support and some of VSC setup code 
> > > > that was handling a scenario that would never happen(PSR2 
> > > > without Y coordinate).
> > > > 
> > > > 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>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -  
> > > > drivers/gpu/drm/i915/intel_psr.c | 46 
> > > > +++++++++++++++++-----------
> > > > ------------
> > > >  2 files changed, 19 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > > b/drivers/gpu/drm/i915/i915_drv.h index 
> > > > 7fe00509e51a..cce32686fd75 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -603,7 +603,6 @@ struct i915_psr {
> > > >  	unsigned busy_frontbuffer_bits;
> > > >  	bool psr2_support;
> > > >  	bool link_standby;
> > > > -	bool y_cord_support;
> > > >  	bool colorimetry_support;
> > > >  	bool alpm;
> > > >  	bool has_hw_tracking;
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d46320a451d9..23f38ab10636 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct 
> > > > intel_dp
> > > > *intel_dp)
> > > >  	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));  
> > > > }
> > > >  
> > > > -static bool intel_dp_get_y_cord_status(struct intel_dp 
> > > > *intel_dp)
> > > > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > > > *intel_dp)
> > > >  {
> > > >  	uint8_t psr_caps = 0;
> > > >  
> > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > > > >psr_dpcd,
> > > >  			 sizeof(intel_dp->psr_dpcd));
> > > >  
> > > > -	if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > > > +	if (intel_dp->psr_dpcd[0]) {
> > > >  		dev_priv->psr.sink_support = true;
> > > >  		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > > >  	}
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > > > -	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > > -
> > > > -		dev_priv->psr.sink_support = true;
> > > > -		dev_priv->psr.psr2_support = true;
> > > > +	    (intel_dp->psr_dpcd[0] ==
> > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > > > +		/*
> > > > +		 * All panels that supports PSR version 03h (PSR2
> > > > +
> > > > +		 * Y-coordinate) can handle Y-coordinates in VSC
> > > > but we are
> > > > +		 * only sure that it is going to be used when
> > > > required by the
> > > > +		 * panel. This way panel is capable to do
> > > > selective update
> > > > +		 * without a aux frame sync.
> > > 
> > > Can you cite this from the spec please? just the "panel is capable 
> > > to do selective update without a aux frame sync" part. Or you 
> > > could just remove that line so that we can get this patch reviewed 
> > > and merged.
> > 
> > There is no such statement in spec like this, the closest that it 
> > have
> > is:
> > "When the Source device includes the optional Y-coordinate in the 
> > SDP for PSR2 Operation, as described in Table 6-12, the Sink device 
> > can implement a lower-precision GTC Slave  function, as described in 
> > Table 7-1."
> > 
> > But we know that this works by all the previous tests when enabling
> > PSR2 and I also tested it.
> 
> Please do update the comment to say so. Do you know if these tests for
> PSR2 selective update are in IGT? Or were these manual tests?
> 
> Rodrigo/Vathsala, any idea how selective update was and can be tested?

nope :(



> 
> The patch as such makes sense to me
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> if 
> you update the comment.
> 
> 
> > 
> > > 
> > > 
> > > > +		 *
> > > > +		 * To support PSR version 02h and PSR version 03h
> > > > without
> > > > +		 * Y-coordinate requirement panels we would need
> > > > to enable
> > > > +		 * GTC first.
> > > > +		 */
> > > > +		dev_priv->psr.psr2_support =
> > > > intel_dp_get_y_coord_required(intel_dp);
> > > >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > >  			      dev_priv->psr.psr2_support ?
> > > > "supported" : "not supported");
> > > >  
> > > >  		if (dev_priv->psr.psr2_support) {
> > > > -			dev_priv->psr.y_cord_support =
> > > > -				intel_dp_get_y_cord_status(intel_d
> > > > p);
> > > >  			dev_priv->psr.colorimetry_support =
> > > >  				intel_dp_get_colorimetry_status(in
> > > > tel_dp);
> > > >  			dev_priv->psr.alpm =
> > > > @@ -191,16 +198,12 @@ static void hsw_psr_setup_vsc(struct 
> > > > intel_dp *intel_dp,
> > > >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> > > >  		psr_vsc.sdp_header.HB0 = 0;
> > > >  		psr_vsc.sdp_header.HB1 = 0x7;
> > > > -		if (dev_priv->psr.colorimetry_support &&
> > > > -		    dev_priv->psr.y_cord_support) {
> > > > +		if (dev_priv->psr.colorimetry_support) {
> > > >  			psr_vsc.sdp_header.HB2 = 0x5;
> > > >  			psr_vsc.sdp_header.HB3 = 0x13;
> > > > -		} else if (dev_priv->psr.y_cord_support) {
> > > > +		} else {
> > > >  			psr_vsc.sdp_header.HB2 = 0x4;
> > > >  			psr_vsc.sdp_header.HB3 = 0xe;
> > > > -		} else {
> > > > -			psr_vsc.sdp_header.HB2 = 0x3;
> > > > -			psr_vsc.sdp_header.HB3 = 0xc;
> > > >  		}
> > > >  	} else {
> > > >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > > > 3.10 */
> > > > @@ -458,15 +461,6 @@ static bool intel_psr2_config_valid(struct 
> > > > intel_dp *intel_dp,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > > > -	 * After gtc implementation , remove this restriction.
> > > > -	 */
> > > > -	if (!dev_priv->psr.y_cord_support) {
> > > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > > > support Y coordinate\n");
> > > > -		return false;
> > > > -	}
> > > > -
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -566,7 +560,6 @@ static void hsw_psr_enable_source(struct 
> > > > intel_dp *intel_dp,
> > > >  	struct drm_device *dev = dig_port->base.base.dev;
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	enum transcoder cpu_transcoder = crtc_state-
> > > > >cpu_transcoder;
> > > > -	u32 chicken;
> > > >  
> > > >  	psr_aux_io_power_get(intel_dp);
> > > >  
> > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct 
> > > > intel_dp *intel_dp,
> > > >  		hsw_psr_setup_aux(intel_dp);
> > > >  
> > > >  	if (dev_priv->psr.psr2_support) {
> > > > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > -		if (dev_priv->psr.y_cord_support)
> > > > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > > > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > > > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> > > >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > > > chicken);
> > > >  
> > > >  		I915_WRITE(EDP_PSR_DEBUG,
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/12] drm/i915/psr/cnl: Enable Y-coordinate support in source

[José Roberto de Souza]

From: "Souza, Jose" <jose.souza@intel.com>

For Geminilake and Cannonlake+ the Y-coordinate support must be
enabled in PSR2_CTL too.

Spec: 7713 and 7720

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da2f6c623ab2..9c4be6bcd1ef 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3892,6 +3892,8 @@ 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_ENABLE	(1<<25) /* GLK and CNL+ */
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
 #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
 #define   EDP_PSR2_TP2_TIME_500		(0<<8)
@@ -6898,6 +6900,7 @@ enum {
 #define CHICKEN_TRANS_A         0x420c0
 #define CHICKEN_TRANS_B         0x420c4
 #define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
+#define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1<<25) /* GLK and CNL+ */
 #define  DDI_TRAINING_OVERRIDE_ENABLE	(1<<19)
 #define  DDI_TRAINING_OVERRIDE_VALUE	(1<<18)
 #define  DDIE_TRAINING_OVERRIDE_ENABLE	(1<<17) /* CHICKEN_TRANS_A only */
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 23f38ab10636..76e021428e57 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -387,8 +387,10 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
 	 * good enough. */
-	val |= EDP_PSR2_ENABLE |
-		EDP_SU_TRACK_ENABLE;
+	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 (drm_dp_dpcd_readb(&intel_dp->aux,
 				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
@@ -570,8 +572,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 		hsw_psr_setup_aux(intel_dp);
 
 	if (dev_priv->psr.psr2_support) {
-		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
-			      | PSR2_ADD_VERTICAL_LINE_COUNT;
+		u32 chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
+
+		if (INTEL_GEN(dev_priv) == 9 && !IS_GEMINILAKE(dev_priv))
+			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
+				   | PSR2_ADD_VERTICAL_LINE_COUNT);
+
+		else
+			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
 		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
 
 		I915_WRITE(EDP_PSR_DEBUG,
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] drm/i915/psr: Do not override PSR2 sink support

[José Roberto de Souza]

Sink can support our PSR2 requirements but userspace can request
a resolution that PSR2 hardware do not support, in this case it
was overwritten the PSR2 sink support.
Adding another flag here, this way if requested resolution changed
to a value that PSR2 hardware can handle, PSR2 can be enabled.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_psr.c    | 33 +++++++++++++++++----------------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7816cd53100a..16f9977995df 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2630,7 +2630,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		   yesno(work_busy(&dev_priv->psr.work.work)));
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
 		else
 			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
@@ -2678,7 +2678,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
 
 		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cce32686fd75..a367fe5538ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -601,11 +601,12 @@ struct i915_psr {
 	bool active;
 	struct delayed_work work;
 	unsigned busy_frontbuffer_bits;
-	bool psr2_support;
+	bool sink_psr2_support;
 	bool link_standby;
 	bool colorimetry_support;
 	bool alpm;
 	bool has_hw_tracking;
+	bool psr2_enabled;
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 76e021428e57..f73e2734a859 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -148,11 +148,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 		 * Y-coordinate requirement panels we would need to enable
 		 * GTC first.
 		 */
-		dev_priv->psr.psr2_support = intel_dp_get_y_coord_required(intel_dp);
-		DRM_DEBUG_KMS("PSR2 %s on sink",
-			      dev_priv->psr.psr2_support ? "supported" : "not supported");
+		dev_priv->psr.sink_psr2_support =
+				intel_dp_get_y_coord_required(intel_dp);
+		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
+			      ? "supported" : "not supported");
 
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.sink_psr2_support) {
 			dev_priv->psr.colorimetry_support =
 				intel_dp_get_colorimetry_status(intel_dp);
 			dev_priv->psr.alpm =
@@ -193,7 +194,7 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
 	struct edp_vsc_psr psr_vsc;
 
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
 		memset(&psr_vsc, 0, sizeof(psr_vsc));
 		psr_vsc.sdp_header.HB0 = 0;
@@ -266,7 +267,7 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 
 	/* Enable ALPM at sink for psr2 */
-	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
+	if (dev_priv->psr.psr2_enabled && dev_priv->psr.alpm)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_RECEIVER_ALPM_CONFIG,
 				DP_ALPM_ENABLE);
@@ -425,7 +426,7 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
 	 */
 
 	/* psr1 and psr2 are mutually exclusive.*/
-	if (dev_priv->psr.psr2_support)
+	if (dev_priv->psr.psr2_enabled)
 		hsw_activate_psr2(intel_dp);
 	else
 		hsw_activate_psr1(intel_dp);
@@ -445,7 +446,7 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	 * dynamically during PSR enable, and extracted from sink
 	 * caps during eDP detection.
 	 */
-	if (!dev_priv->psr.psr2_support)
+	if (!dev_priv->psr.sink_psr2_support)
 		return false;
 
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
@@ -544,7 +545,7 @@ static void intel_psr_activate(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 (dev_priv->psr.psr2_support)
+	if (dev_priv->psr.psr2_enabled)
 		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
 	else
 		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
@@ -571,7 +572,7 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		hsw_psr_setup_aux(intel_dp);
 
-	if (dev_priv->psr.psr2_support) {
+	if (dev_priv->psr.psr2_enabled) {
 		u32 chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
 
 		if (INTEL_GEN(dev_priv) == 9 && !IS_GEMINILAKE(dev_priv))
@@ -630,7 +631,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		goto unlock;
 	}
 
-	dev_priv->psr.psr2_support = crtc_state->has_psr2;
+	dev_priv->psr.psr2_enabled = crtc_state->has_psr2;
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
@@ -700,7 +701,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			psr_status = EDP_PSR2_STATUS;
 			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
 
@@ -724,7 +725,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 
 		dev_priv->psr.active = false;
 	} else {
-		if (dev_priv->psr.psr2_support)
+		if (dev_priv->psr.psr2_enabled)
 			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
@@ -784,7 +785,7 @@ static void intel_psr_work(struct work_struct *work)
 	 * and be ready for re-enable.
 	 */
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			if (intel_wait_for_register(dev_priv,
 						    EDP_PSR2_STATUS,
 						    EDP_PSR2_STATUS_STATE_MASK,
@@ -843,7 +844,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support) {
+		if (dev_priv->psr.psr2_enabled) {
 			val = I915_READ(EDP_PSR2_CTL);
 			WARN_ON(!(val & EDP_PSR2_ENABLE));
 			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
@@ -1012,7 +1013,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 
 	/* By definition flush = invalidate + flush */
 	if (frontbuffer_bits) {
-		if (dev_priv->psr.psr2_support ||
+		if (dev_priv->psr.psr2_enabled ||
 		    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 			intel_psr_exit(dev_priv);
 		} else {
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/12] drm/i915/psr: Use PSR2 macro for PSR2

[José Roberto de Souza]

Cosmetic change.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9c4be6bcd1ef..e660c8a707cf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3903,8 +3903,9 @@ enum {
 #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
 #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
 #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
-#define   EDP_PSR2_IDLE_MASK		0xf
 #define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a)<<4)
+#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
+#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
 
 #define EDP_PSR2_STATUS			_MMIO(0x6f940)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index f73e2734a859..ad69722c329d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -331,7 +331,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	uint32_t val = EDP_PSR_ENABLE;
 
 	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
-	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
+	val |= idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
 
 	if (IS_HASWELL(dev_priv))
 		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] drm/i915/psr: Use PSR2 macro for PSR2

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:43PM -0700, José Roberto de Souza wrote:
> Cosmetic change.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>


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


> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 3 ++-
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c4be6bcd1ef..e660c8a707cf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3903,8 +3903,9 @@ enum {
>  #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
> -#define   EDP_PSR2_IDLE_MASK		0xf
>  #define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a)<<4)
> +#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> +#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
>  
>  #define EDP_PSR2_STATUS			_MMIO(0x6f940)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index f73e2734a859..ad69722c329d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -331,7 +331,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	uint32_t val = EDP_PSR_ENABLE;
>  
>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> -	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> +	val |= idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
>  
>  	if (IS_HASWELL(dev_priv))
>  		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/12] drm/i915/psr: Cache sink synchronization latency

[José Roberto de Souza]

This value do not change overtime so better cache it than
fetch it every PSR enable.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 28 ++++++++++++++++------------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a367fe5538ae..f79338821081 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -607,6 +607,7 @@ struct i915_psr {
 	bool alpm;
 	bool has_hw_tracking;
 	bool psr2_enabled;
+	u8 sink_sync_latency;
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ad69722c329d..19ee6120d3cd 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -122,6 +122,18 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
 	return alpm_caps & DP_ALPM_CAP;
 }
 
+static u8 intel_dp_get_sink_sync_latency(struct intel_dp *intel_dp)
+{
+	u8 val = 0;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux,
+			      DP_SYNCHRONIZATION_LATENCY_IN_SINK, &val) == 1)
+		val &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
+	else
+		DRM_ERROR("Unable to get sink synchronization latency\n");
+	return val;
+}
+
 void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv =
@@ -158,6 +170,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 				intel_dp_get_colorimetry_status(intel_dp);
 			dev_priv->psr.alpm =
 				intel_dp_get_alpm_status(intel_dp);
+			dev_priv->psr.sink_sync_latency =
+				intel_dp_get_sink_sync_latency(intel_dp);
 		}
 	}
 }
@@ -380,10 +394,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * with the 5 or 6 idle patterns.
 	 */
 	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
-	uint32_t val;
-	uint8_t sink_latency;
-
-	val = idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
+	u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
@@ -393,14 +404,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 		val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux,
-				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
-				&sink_latency) == 1) {
-		sink_latency &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
-	} else {
-		sink_latency = 0;
-	}
-	val |= EDP_PSR2_FRAME_BEFORE_SU(sink_latency + 1);
+	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
 
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
 		val |= EDP_PSR2_TP2_TIME_2500;
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915/psr: Cache sink synchronization latency

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:44PM -0700, José Roberto de Souza wrote:
> This value do not change overtime so better cache it than
> fetch it every PSR enable.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 28 ++++++++++++++++------------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a367fe5538ae..f79338821081 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -607,6 +607,7 @@ struct i915_psr {
>  	bool alpm;
>  	bool has_hw_tracking;
>  	bool psr2_enabled;
> +	u8 sink_sync_latency;
>  
>  	void (*enable_source)(struct intel_dp *,
>  			      const struct intel_crtc_state *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index ad69722c329d..19ee6120d3cd 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -122,6 +122,18 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
>  	return alpm_caps & DP_ALPM_CAP;
>  }
>  
> +static u8 intel_dp_get_sink_sync_latency(struct intel_dp *intel_dp)
> +{
> +	u8 val = 0;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> +			      DP_SYNCHRONIZATION_LATENCY_IN_SINK, &val) == 1)
> +		val &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
> +	else
> +		DRM_ERROR("Unable to get sink synchronization latency\n");
> +	return val;
> +}
> +
>  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -158,6 +170,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  				intel_dp_get_colorimetry_status(intel_dp);
>  			dev_priv->psr.alpm =
>  				intel_dp_get_alpm_status(intel_dp);
> +			dev_priv->psr.sink_sync_latency =
> +				intel_dp_get_sink_sync_latency(intel_dp);
>  		}
>  	}
>  }
> @@ -380,10 +394,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 * with the 5 or 6 idle patterns.
>  	 */
>  	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> -	uint32_t val;
> -	uint8_t sink_latency;
> -
> -	val = idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> +	u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;

This belongs to the previous patch apparently. With this moved there keep my rv-b there
and add my rv-b here...

>  
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
> @@ -393,14 +404,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  		val |= EDP_Y_COORDINATE_VALID | EDP_Y_COORDINATE_ENABLE;
>  	}
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> -				&sink_latency) == 1) {
> -		sink_latency &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
> -	} else {
> -		sink_latency = 0;
> -	}
> -	val |= EDP_PSR2_FRAME_BEFORE_SU(sink_latency + 1);
> +	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>  		val |= EDP_PSR2_TP2_TIME_2500;
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915/psr: Cache sink synchronization latency

[Souza, Jose]

On Thu, 2018-03-22 at 16:15 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:44PM -0700, José Roberto de Souza
> wrote:
> > This value do not change overtime so better cache it than
> > fetch it every PSR enable.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 28 ++++++++++++++++------------
> >  2 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index a367fe5538ae..f79338821081 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -607,6 +607,7 @@ struct i915_psr {
> >  	bool alpm;
> >  	bool has_hw_tracking;
> >  	bool psr2_enabled;
> > +	u8 sink_sync_latency;
> >  
> >  	void (*enable_source)(struct intel_dp *,
> >  			      const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index ad69722c329d..19ee6120d3cd 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -122,6 +122,18 @@ static bool intel_dp_get_alpm_status(struct
> > intel_dp *intel_dp)
> >  	return alpm_caps & DP_ALPM_CAP;
> >  }
> >  
> > +static u8 intel_dp_get_sink_sync_latency(struct intel_dp
> > *intel_dp)
> > +{
> > +	u8 val = 0;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +			      DP_SYNCHRONIZATION_LATENCY_IN_SINK,
> > &val) == 1)
> > +		val &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
> > +	else
> > +		DRM_ERROR("Unable to get sink synchronization
> > latency\n");
> > +	return val;
> > +}
> > +
> >  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > @@ -158,6 +170,8 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  				intel_dp_get_colorimetry_status(in
> > tel_dp);
> >  			dev_priv->psr.alpm =
> >  				intel_dp_get_alpm_status(intel_dp)
> > ;
> > +			dev_priv->psr.sink_sync_latency =
> > +				intel_dp_get_sink_sync_latency(int
> > el_dp);
> >  		}
> >  	}
> >  }
> > @@ -380,10 +394,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	 * with the 5 or 6 idle patterns.
> >  	 */
> >  	uint32_t idle_frames = max(6, dev_priv-
> > >vbt.psr.idle_frames);
> > -	uint32_t val;
> > -	uint8_t sink_latency;
> > -
> > -	val = idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> > +	u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> 
> This belongs to the previous patch apparently. With this moved there
> keep my rv-b there
> and add my rv-b here...

Good catch, fixed.
Thanks

> 
> >  
> >  	/* FIXME: selective update is probably totally broken
> > because it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw
> > alone isn't
> > @@ -393,14 +404,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  		val |= EDP_Y_COORDINATE_VALID |
> > EDP_Y_COORDINATE_ENABLE;
> >  	}
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				DP_SYNCHRONIZATION_LATENCY_IN_SINK
> > ,
> > -				&sink_latency) == 1) {
> > -		sink_latency &= DP_MAX_RESYNC_FRAME_COUNT_MASK;
> > -	} else {
> > -		sink_latency = 0;
> > -	}
> > -	val |= EDP_PSR2_FRAME_BEFORE_SU(sink_latency + 1);
> > +	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-
> > >psr.sink_sync_latency + 1);
> >  
> >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> >  		val |= EDP_PSR2_TP2_TIME_2500;
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/12] drm/i915/psr: Set DPCD PSR2 enable bit when needed

[José Roberto de Souza]

In the 2 eDP1.4a pannels tested set or not set bit have no effect
but is better set it and comply with specification.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 19ee6120d3cd..f5c3bcafde25 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -278,19 +278,19 @@ static void hsw_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 = to_i915(dev);
-
+	u8 dpcd_val = DP_PSR_ENABLE;
 
 	/* Enable ALPM at sink for psr2 */
 	if (dev_priv->psr.psr2_enabled && dev_priv->psr.alpm)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_RECEIVER_ALPM_CONFIG,
 				DP_ALPM_ENABLE);
+
+	if (dev_priv->psr.psr2_enabled)
+		dpcd_val |= DP_PSR_ENABLE_PSR2;
 	if (dev_priv->psr.link_standby)
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
-	else
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE);
+		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
 
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
 }
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915/psr: Set DPCD PSR2 enable bit when needed

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:45PM -0700, José Roberto de Souza wrote:
> In the 2 eDP1.4a pannels tested set or not set bit have no effect
> but is better set it and comply with specification.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

looking at the spec it seems that we never *really* had enabled PSR2! =|

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

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 19ee6120d3cd..f5c3bcafde25 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -278,19 +278,19 @@ static void hsw_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 = to_i915(dev);
> -
> +	u8 dpcd_val = DP_PSR_ENABLE;
>  
>  	/* Enable ALPM at sink for psr2 */
>  	if (dev_priv->psr.psr2_enabled && dev_priv->psr.alpm)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  				DP_RECEIVER_ALPM_CONFIG,
>  				DP_ALPM_ENABLE);
> +
> +	if (dev_priv->psr.psr2_enabled)
> +		dpcd_val |= DP_PSR_ENABLE_PSR2;
>  	if (dev_priv->psr.link_standby)
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> -				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
> -	else
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> -				   DP_PSR_ENABLE);
> +		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
>  
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
>  }
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

[José Roberto de Souza]

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 54 +++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 16f9977995df..0a0642c61cd0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
 	return "unknown";
 }
 
+static const char *psr_sink_self_refresh_status(u8 val)
+{
+	static const char * const sink_status[] = {
+		"inactive",
+		"transitioning to active",
+		"active",
+		"active receiving selective update",
+		"transitioning to inactive",
+		"reserved",
+		"reserved",
+		"sink internal error"
+	};
+
+	val &= DP_PSR_SINK_STATE_MASK;
+	if (val < ARRAY_SIZE(sink_status))
+		return sink_status[val];
+
+	return "unknown";
+}
+
+static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val)
+{
+	if (val & DP_PSR_STATE_BIT)
+		seq_puts(m, "\tPSR active\n");
+	if (val & DP_UPDATE_RFB_BIT)
+		seq_puts(m, "\tUpdate RFB\n");
+	if (val & DP_CRC_VALID_BIT)
+		seq_puts(m, "\tCRC valid\n");
+	if (val & DP_SU_VALID)
+		seq_puts(m, "\tSU valid\n");
+	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
+		seq_puts(m, "\tFirst scan line of the SU region\n");
+	if (val & DP_LAST_SCAN_LINE_SU_REGION)
+		seq_puts(m, "\tLast scan line of the SU region\n");
+	if (val & DP_Y_COORDINATE_VALID)
+		seq_puts(m, "\tY-Coordinate valid\n");
+}
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
 			   psr2, psr2_live_status(psr2));
 	}
+
+	if (dev_priv->psr.enabled) {
+		struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux;
+		u8 val;
+
+		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1)
+			seq_printf(m, "Sink self refresh status: 0x%x [%s]\n",
+				   val, psr_sink_self_refresh_status(val));
+
+		if (drm_dp_dpcd_readb(aux, DP_LAST_RECEIVED_PSR_SDP, &val)
+		    == 1) {
+			seq_printf(m, "Sink last received PSR SDP: 0x%x\n",
+				   val);
+			psr_sink_last_received_psr_sdp_sprintf(m, val);
+		}
+	}
 	mutex_unlock(&dev_priv->psr.lock);
 
 	intel_runtime_pm_put(dev_priv);
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza wrote:

please add some justification on why this is useful....

> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 54 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 16f9977995df..0a0642c61cd0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
>  	return "unknown";
>  }
>  
> +static const char *psr_sink_self_refresh_status(u8 val)
> +{
> +	static const char * const sink_status[] = {
> +		"inactive",
> +		"transitioning to active",
> +		"active",
> +		"active receiving selective update",
> +		"transitioning to inactive",
> +		"reserved",
> +		"reserved",
> +		"sink internal error"
> +	};
> +
> +	val &= DP_PSR_SINK_STATE_MASK;
> +	if (val < ARRAY_SIZE(sink_status))
> +		return sink_status[val];
> +
> +	return "unknown";
> +}
> +
> +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val)
> +{
> +	if (val & DP_PSR_STATE_BIT)
> +		seq_puts(m, "\tPSR active\n");

I'm a bit confused here why we are printing status here again if we are adding the
sink_status char * array with some status definition up there.

Any simplification possible?

> +	if (val & DP_UPDATE_RFB_BIT)
> +		seq_puts(m, "\tUpdate RFB\n");
> +	if (val & DP_CRC_VALID_BIT)
> +		seq_puts(m, "\tCRC valid\n");
> +	if (val & DP_SU_VALID)
> +		seq_puts(m, "\tSU valid\n");
> +	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
> +		seq_puts(m, "\tFirst scan line of the SU region\n");
> +	if (val & DP_LAST_SCAN_LINE_SU_REGION)
> +		seq_puts(m, "\tLast scan line of the SU region\n");
> +	if (val & DP_Y_COORDINATE_VALID)
> +		seq_puts(m, "\tY-Coordinate valid\n");
> +}
> +
>  static int i915_edp_psr_status(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
>  			   psr2, psr2_live_status(psr2));
>  	}
> +
> +	if (dev_priv->psr.enabled) {
> +		struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux;
> +		u8 val;
> +
> +		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1)
> +			seq_printf(m, "Sink self refresh status: 0x%x [%s]\n",
> +				   val, psr_sink_self_refresh_status(val));
> +
> +		if (drm_dp_dpcd_readb(aux, DP_LAST_RECEIVED_PSR_SDP, &val)
> +		    == 1) {
> +			seq_printf(m, "Sink last received PSR SDP: 0x%x\n",
> +				   val);
> +			psr_sink_last_received_psr_sdp_sprintf(m, val);
> +		}
> +	}
>  	mutex_unlock(&dev_priv->psr.lock);
>  
>  	intel_runtime_pm_put(dev_priv);
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

[Souza, Jose]

On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza
> wrote:
> 
> please add some justification on why this is useful....

Okay something like this should be fine?

IGT tests could be improved with sink status, knowing for sure that
hardware have activate PSR before get the CRC.
This is also userful to check if hardware is really doing PSR2
selective update with the y-coordinate.


> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 54
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 16f9977995df..0a0642c61cd0 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
> >  	return "unknown";
> >  }
> >  
> > +static const char *psr_sink_self_refresh_status(u8 val)
> > +{
> > +	static const char * const sink_status[] = {
> > +		"inactive",
> > +		"transitioning to active",
> > +		"active",
> > +		"active receiving selective update",
> > +		"transitioning to inactive",
> > +		"reserved",
> > +		"reserved",
> > +		"sink internal error"
> > +	};
> > +
> > +	val &= DP_PSR_SINK_STATE_MASK;
> > +	if (val < ARRAY_SIZE(sink_status))
> > +		return sink_status[val];
> > +
> > +	return "unknown";
> > +}
> > +
> > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file
> > *m, u32 val)
> > +{
> > +	if (val & DP_PSR_STATE_BIT)
> > +		seq_puts(m, "\tPSR active\n");
> 
> I'm a bit confused here why we are printing status here again if we
> are adding the
> sink_status char * array with some status definition up there.
> 
> Any simplification possible?

Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the
status of the sink to change, so I this 2 can be removed.

> 
> > +	if (val & DP_UPDATE_RFB_BIT)
> > +		seq_puts(m, "\tUpdate RFB\n");
> > +	if (val & DP_CRC_VALID_BIT)
> > +		seq_puts(m, "\tCRC valid\n");
> > +	if (val & DP_SU_VALID)
> > +		seq_puts(m, "\tSU valid\n");
> > +	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
> > +		seq_puts(m, "\tFirst scan line of the SU
> > region\n");
> > +	if (val & DP_LAST_SCAN_LINE_SU_REGION)
> > +		seq_puts(m, "\tLast scan line of the SU
> > region\n");
> > +	if (val & DP_Y_COORDINATE_VALID)
> > +		seq_puts(m, "\tY-Coordinate valid\n");
> > +}
> > +
> >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > >private);
> > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> >  			   psr2, psr2_live_status(psr2));
> >  	}
> > +
> > +	if (dev_priv->psr.enabled) {
> > +		struct drm_dp_aux *aux = &dev_priv->psr.enabled-
> > >aux;
> > +		u8 val;
> > +
> > +		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) ==
> > 1)
> > +			seq_printf(m, "Sink self refresh status:
> > 0x%x [%s]\n",
> > +				   val,
> > psr_sink_self_refresh_status(val));
> > +
> > +		if (drm_dp_dpcd_readb(aux,
> > DP_LAST_RECEIVED_PSR_SDP, &val)
> > +		    == 1) {
> > +			seq_printf(m, "Sink last received PSR SDP:
> > 0x%x\n",
> > +				   val);
> > +			psr_sink_last_received_psr_sdp_sprintf(m,
> > val);
> > +		}
> > +	}
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  
> >  	intel_runtime_pm_put(dev_priv);
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 05:06:13PM -0700, Souza, Jose wrote:
> On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote:
> > On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza
> > wrote:
> > 
> > please add some justification on why this is useful....
> 
> Okay something like this should be fine?
> 
> IGT tests could be improved with sink status, knowing for sure that
> hardware have activate PSR before get the CRC.
> This is also userful to check if hardware is really doing PSR2
> selective update with the y-coordinate.

I think so. Although for the tests I'd like you sync with DK on this
versus the Interrupt approach.

> 
> 
> > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 54
> > > +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 16f9977995df..0a0642c61cd0 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
> > >  	return "unknown";
> > >  }
> > >  
> > > +static const char *psr_sink_self_refresh_status(u8 val)
> > > +{
> > > +	static const char * const sink_status[] = {
> > > +		"inactive",
> > > +		"transitioning to active",
> > > +		"active",
> > > +		"active receiving selective update",
> > > +		"transitioning to inactive",
> > > +		"reserved",
> > > +		"reserved",
> > > +		"sink internal error"
> > > +	};
> > > +
> > > +	val &= DP_PSR_SINK_STATE_MASK;
> > > +	if (val < ARRAY_SIZE(sink_status))
> > > +		return sink_status[val];
> > > +
> > > +	return "unknown";
> > > +}
> > > +
> > > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file
> > > *m, u32 val)
> > > +{
> > > +	if (val & DP_PSR_STATE_BIT)
> > > +		seq_puts(m, "\tPSR active\n");
> > 
> > I'm a bit confused here why we are printing status here again if we
> > are adding the
> > sink_status char * array with some status definition up there.
> > 
> > Any simplification possible?
> 
> Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the
> status of the sink to change, so I this 2 can be removed.
> 
> > 
> > > +	if (val & DP_UPDATE_RFB_BIT)
> > > +		seq_puts(m, "\tUpdate RFB\n");
> > > +	if (val & DP_CRC_VALID_BIT)
> > > +		seq_puts(m, "\tCRC valid\n");
> > > +	if (val & DP_SU_VALID)
> > > +		seq_puts(m, "\tSU valid\n");
> > > +	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
> > > +		seq_puts(m, "\tFirst scan line of the SU
> > > region\n");
> > > +	if (val & DP_LAST_SCAN_LINE_SU_REGION)
> > > +		seq_puts(m, "\tLast scan line of the SU
> > > region\n");
> > > +	if (val & DP_Y_COORDINATE_VALID)
> > > +		seq_puts(m, "\tY-Coordinate valid\n");
> > > +}
> > > +
> > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  {
> > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > >private);
> > > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct
> > > seq_file *m, void *data)
> > >  		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> > >  			   psr2, psr2_live_status(psr2));
> > >  	}
> > > +
> > > +	if (dev_priv->psr.enabled) {
> > > +		struct drm_dp_aux *aux = &dev_priv->psr.enabled-
> > > >aux;
> > > +		u8 val;
> > > +
> > > +		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) ==
> > > 1)
> > > +			seq_printf(m, "Sink self refresh status:
> > > 0x%x [%s]\n",
> > > +				   val,
> > > psr_sink_self_refresh_status(val));
> > > +
> > > +		if (drm_dp_dpcd_readb(aux,
> > > DP_LAST_RECEIVED_PSR_SDP, &val)
> > > +		    == 1) {
> > > +			seq_printf(m, "Sink last received PSR SDP:
> > > 0x%x\n",
> > > +				   val);
> > > +			psr_sink_last_received_psr_sdp_sprintf(m,
> > > val);
> > > +		}
> > > +	}
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  
> > >  	intel_runtime_pm_put(dev_priv);
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

[Pandiyan, Dhinakaran]

On Fri, 2018-03-23 at 00:06 +0000, Souza, Jose wrote:
> On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote:
> > On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza
> > wrote:
> > 
> > please add some justification on why this is useful....
> 

The plan is to remove sink crc and use these bits :)

> Okay something like this should be fine?
> 
> IGT tests could be improved with sink status, knowing for sure that
> hardware have activate PSR before get the CRC.
> This is also userful to check if hardware is really doing PSR2
> selective update with the y-coordinate.
> 
> 
> > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 54
> > > +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 54 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 16f9977995df..0a0642c61cd0 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
> > >  	return "unknown";
> > >  }
> > >  
> > > +static const char *psr_sink_self_refresh_status(u8 val)
> > > +{
> > > +	static const char * const sink_status[] = {
> > > +		"inactive",
> > > +		"transitioning to active",
> > > +		"active",
> > > +		"active receiving selective update",
> > > +		"transitioning to inactive",
> > > +		"reserved",
> > > +		"reserved",
> > > +		"sink internal error"
> > > +	};
> > > +
> > > +	val &= DP_PSR_SINK_STATE_MASK;
> > > +	if (val < ARRAY_SIZE(sink_status))
> > > +		return sink_status[val];
> > > +
> > > +	return "unknown";
> > > +}
> > > +
> > > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file
> > > *m, u32 val)
> > > +{
> > > +	if (val & DP_PSR_STATE_BIT)
> > > +		seq_puts(m, "\tPSR active\n");
> > 
> > I'm a bit confused here why we are printing status here again if we
> > are adding the
> > sink_status char * array with some status definition up there.
> > 
> > Any simplification possible?

These values are what the sink received in the SDP, the one from
DP_PSR_STATUS is the current status of the sink.

> 
> Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the
> status of the sink to change, so I this 2 can be removed.

This should be split as two patches. And more importantly I can't think
of a use case for printing SDP status. The update might be too fast for
making any sense of it. Printing DP_PSR_STATUS should be sufficient for
writing tests.

> 
> > 
> > > +	if (val & DP_UPDATE_RFB_BIT)
> > > +		seq_puts(m, "\tUpdate RFB\n");
> > > +	if (val & DP_CRC_VALID_BIT)
> > > +		seq_puts(m, "\tCRC valid\n");
> > > +	if (val & DP_SU_VALID)
> > > +		seq_puts(m, "\tSU valid\n");
> > > +	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
> > > +		seq_puts(m, "\tFirst scan line of the SU
> > > region\n");
> > > +	if (val & DP_LAST_SCAN_LINE_SU_REGION)
> > > +		seq_puts(m, "\tLast scan line of the SU
> > > region\n");
> > > +	if (val & DP_Y_COORDINATE_VALID)
> > > +		seq_puts(m, "\tY-Coordinate valid\n");
> > > +}
> > > +
> > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  {
> > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > >private);
> > > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct
> > > seq_file *m, void *data)
> > >  		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> > >  			   psr2, psr2_live_status(psr2));
> > >  	}
> > > +
> > > +	if (dev_priv->psr.enabled) {
> > > +		struct drm_dp_aux *aux = &dev_priv->psr.enabled-
> > > >aux;
> > > +		u8 val;
> > > +
> > > +		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) ==
> > > 1)
> > > +			seq_printf(m, "Sink self refresh status:
> > > 0x%x [%s]\n",
> > > +				   val,
> > > psr_sink_self_refresh_status(val));
> > > +
> > > +		if (drm_dp_dpcd_readb(aux,
> > > DP_LAST_RECEIVED_PSR_SDP, &val)
> > > +		    == 1) {
> > > +			seq_printf(m, "Sink last received PSR SDP:
> > > 0x%x\n",
> > > +				   val);
> > > +			psr_sink_last_received_psr_sdp_sprintf(m,
> > > val);
> > > +		}
> > > +	}
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  
> > >  	intel_runtime_pm_put(dev_priv);
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] drm/i915/debugfs: Print information about what caused a PSR exit

[José Roberto de Souza]

This will be helpful to debug what hardware is actually tracking.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 47 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     | 18 ++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0a0642c61cd0..3182e9a7cc5d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2641,6 +2641,43 @@ static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val)
 		seq_puts(m, "\tY-Coordinate valid\n");
 }
 
+static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
+				   bool psr2_enabled)
+{
+	if (val & EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE)
+		seq_puts(m, "\tPSR2 watchdog timer expired\n");
+	if ((val & EDP_PSR_EVENT_PSR2_DISABLED) && psr2_enabled)
+		seq_puts(m, "\tPSR2 disabled\n");
+	if (val & EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN)
+		seq_puts(m, "\tSU dirty FIFO underrun\n");
+	if (val & EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN)
+		seq_puts(m, "\tSU CRC FIFO underrun\n");
+	if (val & EDP_PSR_EVENT_GRAPHICS_RESET)
+		seq_puts(m, "\tGraphics reset\n");
+	if (val & EDP_PSR_EVENT_PCH_INTERRUPT)
+		seq_puts(m, "\tPCH interrupt\n");
+	if (val & EDP_PSR_EVENT_MEMORY_UP)
+		seq_puts(m, "\tMemory up\n");
+	if (val & EDP_PSR_EVENT_FRONT_BUFFER_MODIFY)
+		seq_puts(m, "\tFront buffer modification\n");
+	if (val & EDP_PSR_EVENT_WD_TIMER_EXPIRE)
+		seq_puts(m, "\tPSR watchdog timer expired\n");
+	if (val & EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE)
+		seq_puts(m, "\tPIPE registers updated\n");
+	if (val & EDP_PSR_EVENT_REGISTER_UPDATE)
+		seq_puts(m, "\tRegister update\n");
+	if (val & EDP_PSR_EVENT_HDCP_ENABLE)
+		seq_puts(m, "\tHDCP enabled\n");
+	if (val & EDP_PSR_EVENT_KVMR_SESSION_ENABLE)
+		seq_puts(m, "\tKVMR session enabled\n");
+	if (val & EDP_PSR_EVENT_VBI_ENABLE)
+		seq_puts(m, "\tVBI enabled\n");
+	if (val & EDP_PSR_EVENT_LPSP_MODE_EXIT)
+		seq_puts(m, "\tLPSP mode exited\n");
+	if ((val & EDP_PSR_EVENT_PSR_DISABLE) && !psr2_enabled)
+		seq_puts(m, "\tPSR disabled\n");
+}
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -2716,6 +2753,16 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
+
+	if (INTEL_GEN(dev_priv) >= 9) {
+		u32 val = I915_READ(EDP_PSR_EVENT);
+
+		seq_printf(m, "Event triggered PSR exit: 0x%x\n", val);
+		psr_event_exit_sprintf(m, val, dev_priv->psr.psr2_enabled);
+		/* clean events */
+		I915_WRITE(EDP_PSR_EVENT, val);
+	}
+
 	if (dev_priv->psr.psr2_enabled) {
 		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e660c8a707cf..45f7703a9ee6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3907,6 +3907,24 @@ enum {
 #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
 #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
 
+#define EDP_PSR_EVENT				_MMIO(0x6f848)
+#define  EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE	(1 << 17)
+#define  EDP_PSR_EVENT_PSR2_DISABLED		(1 << 16)
+#define  EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
+#define  EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN	(1 << 14)
+#define  EDP_PSR_EVENT_GRAPHICS_RESET		(1 << 12)
+#define  EDP_PSR_EVENT_PCH_INTERRUPT		(1 << 11)
+#define  EDP_PSR_EVENT_MEMORY_UP		(1 << 10)
+#define  EDP_PSR_EVENT_FRONT_BUFFER_MODIFY	(1 << 9)
+#define  EDP_PSR_EVENT_WD_TIMER_EXPIRE		(1 << 8)
+#define  EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE	(1 << 6)
+#define  EDP_PSR_EVENT_REGISTER_UPDATE		(1 << 5)
+#define  EDP_PSR_EVENT_HDCP_ENABLE		(1 << 4)
+#define  EDP_PSR_EVENT_KVMR_SESSION_ENABLE	(1 << 3)
+#define  EDP_PSR_EVENT_VBI_ENABLE		(1 << 2)
+#define  EDP_PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
+#define  EDP_PSR_EVENT_PSR_DISABLE		(1 << 0)
+
 #define EDP_PSR2_STATUS			_MMIO(0x6f940)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915/debugfs: Print information about what caused a PSR exit

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:47PM -0700, José Roberto de Souza wrote:
> This will be helpful to debug what hardware is actually tracking.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 47 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     | 18 ++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0a0642c61cd0..3182e9a7cc5d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2641,6 +2641,43 @@ static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val)
>  		seq_puts(m, "\tY-Coordinate valid\n");
>  }
>  
> +static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
> +				   bool psr2_enabled)
> +{
> +	if (val & EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE)
> +		seq_puts(m, "\tPSR2 watchdog timer expired\n");
> +	if ((val & EDP_PSR_EVENT_PSR2_DISABLED) && psr2_enabled)
> +		seq_puts(m, "\tPSR2 disabled\n");
> +	if (val & EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN)
> +		seq_puts(m, "\tSU dirty FIFO underrun\n");
> +	if (val & EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN)
> +		seq_puts(m, "\tSU CRC FIFO underrun\n");
> +	if (val & EDP_PSR_EVENT_GRAPHICS_RESET)
> +		seq_puts(m, "\tGraphics reset\n");
> +	if (val & EDP_PSR_EVENT_PCH_INTERRUPT)
> +		seq_puts(m, "\tPCH interrupt\n");
> +	if (val & EDP_PSR_EVENT_MEMORY_UP)
> +		seq_puts(m, "\tMemory up\n");
> +	if (val & EDP_PSR_EVENT_FRONT_BUFFER_MODIFY)
> +		seq_puts(m, "\tFront buffer modification\n");
> +	if (val & EDP_PSR_EVENT_WD_TIMER_EXPIRE)
> +		seq_puts(m, "\tPSR watchdog timer expired\n");
> +	if (val & EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE)
> +		seq_puts(m, "\tPIPE registers updated\n");
> +	if (val & EDP_PSR_EVENT_REGISTER_UPDATE)
> +		seq_puts(m, "\tRegister update\n");
> +	if (val & EDP_PSR_EVENT_HDCP_ENABLE)
> +		seq_puts(m, "\tHDCP enabled\n");
> +	if (val & EDP_PSR_EVENT_KVMR_SESSION_ENABLE)
> +		seq_puts(m, "\tKVMR session enabled\n");
> +	if (val & EDP_PSR_EVENT_VBI_ENABLE)
> +		seq_puts(m, "\tVBI enabled\n");
> +	if (val & EDP_PSR_EVENT_LPSP_MODE_EXIT)
> +		seq_puts(m, "\tLPSP mode exited\n");
> +	if ((val & EDP_PSR_EVENT_PSR_DISABLE) && !psr2_enabled)
> +		seq_puts(m, "\tPSR disabled\n");
> +}
> +
>  static int i915_edp_psr_status(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -2716,6 +2753,16 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  		seq_printf(m, "Performance_Counter: %u\n", psrperf);
>  	}
> +
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		u32 val = I915_READ(EDP_PSR_EVENT);

What I'm afraid here is that this really shows the last event or the first one after we cleared.

> +
> +		seq_printf(m, "Event triggered PSR exit: 0x%x\n", val);
> +		psr_event_exit_sprintf(m, val, dev_priv->psr.psr2_enabled);
> +		/* clean events */
> +		I915_WRITE(EDP_PSR_EVENT, val);

And to clear do we really need to set the bits or clear the bits?

> +	}
> +
>  	if (dev_priv->psr.psr2_enabled) {
>  		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e660c8a707cf..45f7703a9ee6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3907,6 +3907,24 @@ enum {
>  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
>  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
>  
> +#define EDP_PSR_EVENT				_MMIO(0x6f848)
> +#define  EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE	(1 << 17)
> +#define  EDP_PSR_EVENT_PSR2_DISABLED		(1 << 16)
> +#define  EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> +#define  EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN	(1 << 14)
> +#define  EDP_PSR_EVENT_GRAPHICS_RESET		(1 << 12)
> +#define  EDP_PSR_EVENT_PCH_INTERRUPT		(1 << 11)
> +#define  EDP_PSR_EVENT_MEMORY_UP		(1 << 10)
> +#define  EDP_PSR_EVENT_FRONT_BUFFER_MODIFY	(1 << 9)
> +#define  EDP_PSR_EVENT_WD_TIMER_EXPIRE		(1 << 8)
> +#define  EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE	(1 << 6)
> +#define  EDP_PSR_EVENT_REGISTER_UPDATE		(1 << 5)
> +#define  EDP_PSR_EVENT_HDCP_ENABLE		(1 << 4)
> +#define  EDP_PSR_EVENT_KVMR_SESSION_ENABLE	(1 << 3)
> +#define  EDP_PSR_EVENT_VBI_ENABLE		(1 << 2)
> +#define  EDP_PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> +#define  EDP_PSR_EVENT_PSR_DISABLE		(1 << 0)
> +
>  #define EDP_PSR2_STATUS			_MMIO(0x6f940)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915/debugfs: Print information about what caused a PSR exit

[Pandiyan, Dhinakaran]

On Thu, 2018-03-22 at 16:27 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:47PM -0700, José Roberto de Souza wrote:
> > This will be helpful to debug what hardware is actually tracking.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 47 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h     | 18 ++++++++++++++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 0a0642c61cd0..3182e9a7cc5d 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2641,6 +2641,43 @@ static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val)
> >  		seq_puts(m, "\tY-Coordinate valid\n");
> >  }
> >  
> > +static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
> > +				   bool psr2_enabled)
> > +{
> > +	if (val & EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE)
> > +		seq_puts(m, "\tPSR2 watchdog timer expired\n");
> > +	if ((val & EDP_PSR_EVENT_PSR2_DISABLED) && psr2_enabled)
> > +		seq_puts(m, "\tPSR2 disabled\n");
> > +	if (val & EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN)
> > +		seq_puts(m, "\tSU dirty FIFO underrun\n");
> > +	if (val & EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN)
> > +		seq_puts(m, "\tSU CRC FIFO underrun\n");
> > +	if (val & EDP_PSR_EVENT_GRAPHICS_RESET)
> > +		seq_puts(m, "\tGraphics reset\n");
> > +	if (val & EDP_PSR_EVENT_PCH_INTERRUPT)
> > +		seq_puts(m, "\tPCH interrupt\n");
> > +	if (val & EDP_PSR_EVENT_MEMORY_UP)
> > +		seq_puts(m, "\tMemory up\n");
> > +	if (val & EDP_PSR_EVENT_FRONT_BUFFER_MODIFY)
> > +		seq_puts(m, "\tFront buffer modification\n");
> > +	if (val & EDP_PSR_EVENT_WD_TIMER_EXPIRE)
> > +		seq_puts(m, "\tPSR watchdog timer expired\n");
> > +	if (val & EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE)
> > +		seq_puts(m, "\tPIPE registers updated\n");
> > +	if (val & EDP_PSR_EVENT_REGISTER_UPDATE)
> > +		seq_puts(m, "\tRegister update\n");
> > +	if (val & EDP_PSR_EVENT_HDCP_ENABLE)
> > +		seq_puts(m, "\tHDCP enabled\n");
> > +	if (val & EDP_PSR_EVENT_KVMR_SESSION_ENABLE)
> > +		seq_puts(m, "\tKVMR session enabled\n");
> > +	if (val & EDP_PSR_EVENT_VBI_ENABLE)
> > +		seq_puts(m, "\tVBI enabled\n");
> > +	if (val & EDP_PSR_EVENT_LPSP_MODE_EXIT)
> > +		seq_puts(m, "\tLPSP mode exited\n");
> > +	if ((val & EDP_PSR_EVENT_PSR_DISABLE) && !psr2_enabled)
> > +		seq_puts(m, "\tPSR disabled\n");
> > +}
> > +
> >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > @@ -2716,6 +2753,16 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  
> >  		seq_printf(m, "Performance_Counter: %u\n", psrperf);
> >  	}
> > +
> > +	if (INTEL_GEN(dev_priv) >= 9) {
> > +		u32 val = I915_READ(EDP_PSR_EVENT);
> 
> What I'm afraid here is that this really shows the last event or the first one after we cleared.
> 
Both, the bits remain set unless cleared. I have plans of printing the
events out of the PSR exit irq handler. This really was one of the main
reasons to implement PSR interrupts. Since we get interrupt for each PSR
exit, we'll also print out the correct event that caused exit.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915/debugfs: Print information about what caused a PSR exit

[Souza, Jose]

On Thu, 2018-03-22 at 16:43 -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> On Thu, 2018-03-22 at 16:27 -0700, Rodrigo Vivi wrote:
> > On Thu, Mar 22, 2018 at 02:48:47PM -0700, José Roberto de Souza
> > wrote:
> > > This will be helpful to debug what hardware is actually tracking.
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 47
> > > +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h     | 18 ++++++++++++++
> > >  2 files changed, 65 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 0a0642c61cd0..3182e9a7cc5d 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2641,6 +2641,43 @@ static void
> > > psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32
> > > val)
> > >  		seq_puts(m, "\tY-Coordinate valid\n");
> > >  }
> > >  
> > > +static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
> > > +				   bool psr2_enabled)
> > > +{
> > > +	if (val & EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE)
> > > +		seq_puts(m, "\tPSR2 watchdog timer expired\n");
> > > +	if ((val & EDP_PSR_EVENT_PSR2_DISABLED) && psr2_enabled)
> > > +		seq_puts(m, "\tPSR2 disabled\n");
> > > +	if (val & EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN)
> > > +		seq_puts(m, "\tSU dirty FIFO underrun\n");
> > > +	if (val & EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN)
> > > +		seq_puts(m, "\tSU CRC FIFO underrun\n");
> > > +	if (val & EDP_PSR_EVENT_GRAPHICS_RESET)
> > > +		seq_puts(m, "\tGraphics reset\n");
> > > +	if (val & EDP_PSR_EVENT_PCH_INTERRUPT)
> > > +		seq_puts(m, "\tPCH interrupt\n");
> > > +	if (val & EDP_PSR_EVENT_MEMORY_UP)
> > > +		seq_puts(m, "\tMemory up\n");
> > > +	if (val & EDP_PSR_EVENT_FRONT_BUFFER_MODIFY)
> > > +		seq_puts(m, "\tFront buffer modification\n");
> > > +	if (val & EDP_PSR_EVENT_WD_TIMER_EXPIRE)
> > > +		seq_puts(m, "\tPSR watchdog timer expired\n");
> > > +	if (val & EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE)
> > > +		seq_puts(m, "\tPIPE registers updated\n");
> > > +	if (val & EDP_PSR_EVENT_REGISTER_UPDATE)
> > > +		seq_puts(m, "\tRegister update\n");
> > > +	if (val & EDP_PSR_EVENT_HDCP_ENABLE)
> > > +		seq_puts(m, "\tHDCP enabled\n");
> > > +	if (val & EDP_PSR_EVENT_KVMR_SESSION_ENABLE)
> > > +		seq_puts(m, "\tKVMR session enabled\n");
> > > +	if (val & EDP_PSR_EVENT_VBI_ENABLE)
> > > +		seq_puts(m, "\tVBI enabled\n");
> > > +	if (val & EDP_PSR_EVENT_LPSP_MODE_EXIT)
> > > +		seq_puts(m, "\tLPSP mode exited\n");
> > > +	if ((val & EDP_PSR_EVENT_PSR_DISABLE) && !psr2_enabled)
> > > +		seq_puts(m, "\tPSR disabled\n");
> > > +}
> > > +
> > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  {
> > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > >private);
> > > @@ -2716,6 +2753,16 @@ static int i915_edp_psr_status(struct
> > > seq_file *m, void *data)
> > >  
> > >  		seq_printf(m, "Performance_Counter: %u\n",
> > > psrperf);
> > >  	}
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 9) {
> > > +		u32 val = I915_READ(EDP_PSR_EVENT);
> > 
> > What I'm afraid here is that this really shows the last event or
> > the first one after we cleared.
> > 
> 
> Both, the bits remain set unless cleared. I have plans of printing
> the
> events out of the PSR exit irq handler. This really was one of the
> main
> reasons to implement PSR interrupts. Since we get interrupt for each
> PSR
> exit, we'll also print out the correct event that caused exit.
> 

Exactly that, so should I drop this one?
Are you planning send a patch later showing the reasons in the debugfs?
This way we could improve and write new IGT tests.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915/debugfs: Print information about what caused a PSR exit

[Pandiyan, Dhinakaran]

On Fri, 2018-03-23 at 00:16 +0000, Souza, Jose wrote:
> On Thu, 2018-03-22 at 16:43 -0700, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > On Thu, 2018-03-22 at 16:27 -0700, Rodrigo Vivi wrote:
> > > On Thu, Mar 22, 2018 at 02:48:47PM -0700, José Roberto de Souza
> > > wrote:
> > > > This will be helpful to debug what hardware is actually tracking.
> > > > 
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 47
> > > > +++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/i915_reg.h     | 18 ++++++++++++++
> > > >  2 files changed, 65 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 0a0642c61cd0..3182e9a7cc5d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2641,6 +2641,43 @@ static void
> > > > psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32
> > > > val)
> > > >  		seq_puts(m, "\tY-Coordinate valid\n");
> > > >  }
> > > >  
> > > > +static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
> > > > +				   bool psr2_enabled)
> > > > +{
> > > > +	if (val & EDP_PSR_EVENT_PSR2_WD_TIMER_EXPIRE)
> > > > +		seq_puts(m, "\tPSR2 watchdog timer expired\n");
> > > > +	if ((val & EDP_PSR_EVENT_PSR2_DISABLED) && psr2_enabled)
> > > > +		seq_puts(m, "\tPSR2 disabled\n");
> > > > +	if (val & EDP_PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN)
> > > > +		seq_puts(m, "\tSU dirty FIFO underrun\n");
> > > > +	if (val & EDP_PSR_EVENT_SU_CRC_FIFO_UNDERRUN)
> > > > +		seq_puts(m, "\tSU CRC FIFO underrun\n");
> > > > +	if (val & EDP_PSR_EVENT_GRAPHICS_RESET)
> > > > +		seq_puts(m, "\tGraphics reset\n");
> > > > +	if (val & EDP_PSR_EVENT_PCH_INTERRUPT)
> > > > +		seq_puts(m, "\tPCH interrupt\n");
> > > > +	if (val & EDP_PSR_EVENT_MEMORY_UP)
> > > > +		seq_puts(m, "\tMemory up\n");
> > > > +	if (val & EDP_PSR_EVENT_FRONT_BUFFER_MODIFY)
> > > > +		seq_puts(m, "\tFront buffer modification\n");
> > > > +	if (val & EDP_PSR_EVENT_WD_TIMER_EXPIRE)
> > > > +		seq_puts(m, "\tPSR watchdog timer expired\n");
> > > > +	if (val & EDP_PSR_EVENT_PIPE_REGISTERS_UPDATE)
> > > > +		seq_puts(m, "\tPIPE registers updated\n");
> > > > +	if (val & EDP_PSR_EVENT_REGISTER_UPDATE)
> > > > +		seq_puts(m, "\tRegister update\n");
> > > > +	if (val & EDP_PSR_EVENT_HDCP_ENABLE)
> > > > +		seq_puts(m, "\tHDCP enabled\n");
> > > > +	if (val & EDP_PSR_EVENT_KVMR_SESSION_ENABLE)
> > > > +		seq_puts(m, "\tKVMR session enabled\n");
> > > > +	if (val & EDP_PSR_EVENT_VBI_ENABLE)
> > > > +		seq_puts(m, "\tVBI enabled\n");
> > > > +	if (val & EDP_PSR_EVENT_LPSP_MODE_EXIT)
> > > > +		seq_puts(m, "\tLPSP mode exited\n");
> > > > +	if ((val & EDP_PSR_EVENT_PSR_DISABLE) && !psr2_enabled)
> > > > +		seq_puts(m, "\tPSR disabled\n");
> > > > +}
> > > > +
> > > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > > >private);
> > > > @@ -2716,6 +2753,16 @@ static int i915_edp_psr_status(struct
> > > > seq_file *m, void *data)
> > > >  
> > > >  		seq_printf(m, "Performance_Counter: %u\n",
> > > > psrperf);
> > > >  	}
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 9) {
> > > > +		u32 val = I915_READ(EDP_PSR_EVENT);
> > > 
> > > What I'm afraid here is that this really shows the last event or
> > > the first one after we cleared.
> > > 
> > 
> > Both, the bits remain set unless cleared. I have plans of printing
> > the
> > events out of the PSR exit irq handler. This really was one of the
> > main
> > reasons to implement PSR interrupts. Since we get interrupt for each
> > PSR
> > exit, we'll also print out the correct event that caused exit.
> > 
> 
> Exactly that, so should I drop this one?
> Are you planning send a patch later showing the reasons in the debugfs?

Yeah, I was planning to do it after the interrupt series is merged.
Since you've already done all the work for this patch, I think it makes
sense for you to one this one on top of PSR interrupts.

> This way we could improve and write new IGT tests.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/12] drm/i915/debugfs: Print how many blocks were sent in a selective update

[José Roberto de Souza]

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 40 ++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h     | 17 ++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3182e9a7cc5d..20985584cc0f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2678,6 +2678,43 @@ static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
 		seq_puts(m, "\tPSR disabled\n");
 }
 
+static void psr2_su_blocks_sprintf(struct seq_file *m,
+				   struct drm_i915_private *dev_priv)
+{
+	u32 val;
+	u16 su;
+
+	val = I915_READ(EDP_PSR2_SU_STATUS);
+	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT;
+	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N: %d\n", su);
+	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT;
+	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-1: %d\n", su);
+	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT;
+	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-2: %d\n", su);
+
+	val = I915_READ(EDP_PSR2_SU_STATUS2);
+	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT;
+	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-3: %d\n", su);
+	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT;
+	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-4: %d\n", su);
+	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT;
+	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-5: %d\n", su);
+
+	val = I915_READ(EDP_PSR2_SU_STATUS3);
+	su = val >> EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT;
+	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-6: %d\n", su);
+	su = val >> EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT;
+	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
+	seq_printf(m, "\tSU blocks in frame N-7: %d\n", su);
+}
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -2766,8 +2803,9 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	if (dev_priv->psr.psr2_enabled) {
 		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
 
-		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
+		seq_printf(m, "EDP_PSR2_STATUS: 0x%x [%s]\n",
 			   psr2, psr2_live_status(psr2));
+		psr2_su_blocks_sprintf(m, dev_priv);
 	}
 
 	if (dev_priv->psr.enabled) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 45f7703a9ee6..18af3e8fd4b6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3929,6 +3929,23 @@ enum {
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
+#define EDP_PSR2_SU_STATUS					  _MMIO(0x6F914)
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK			  0x3FF
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT		  0
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT	  10
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT	  20
+
+#define EDP_PSR2_SU_STATUS2					  _MMIO(0x6F918)
+#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK			  0x3FF
+#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT  0
+#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT  10
+#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT  20
+
+#define EDP_PSR2_SU_STATUS3					  _MMIO(0x6F91C)
+#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK			  0x3FF
+#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT  0
+#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT  10
+
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
 #define PCH_ADPA                _MMIO(0xe1100)
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915/debugfs: Print how many blocks were sent in a selective update

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:48PM -0700, José Roberto de Souza wrote:
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 40 ++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h     | 17 ++++++++++++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3182e9a7cc5d..20985584cc0f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2678,6 +2678,43 @@ static void psr_event_exit_sprintf(struct seq_file *m, u32 val,
>  		seq_puts(m, "\tPSR disabled\n");
>  }
>  
> +static void psr2_su_blocks_sprintf(struct seq_file *m,
> +				   struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +	u16 su;
> +
> +	val = I915_READ(EDP_PSR2_SU_STATUS);
> +	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N: %d\n", su);
> +	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-1: %d\n", su);
> +	su = val >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-2: %d\n", su);
> +
> +	val = I915_READ(EDP_PSR2_SU_STATUS2);
> +	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-3: %d\n", su);
> +	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-4: %d\n", su);
> +	su = val >> EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-5: %d\n", su);
> +
> +	val = I915_READ(EDP_PSR2_SU_STATUS3);
> +	su = val >> EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-6: %d\n", su);
> +	su = val >> EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT;
> +	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
> +	seq_printf(m, "\tSU blocks in frame N-7: %d\n", su);
> +}
> +
>  static int i915_edp_psr_status(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -2766,8 +2803,9 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	if (dev_priv->psr.psr2_enabled) {
>  		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
>  
> -		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> +		seq_printf(m, "EDP_PSR2_STATUS: 0x%x [%s]\n",
>  			   psr2, psr2_live_status(psr2));
> +		psr2_su_blocks_sprintf(m, dev_priv);
>  	}
>  
>  	if (dev_priv->psr.enabled) {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 45f7703a9ee6..18af3e8fd4b6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3929,6 +3929,23 @@ enum {
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> +#define EDP_PSR2_SU_STATUS					  _MMIO(0x6F914)
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK			  0x3FF
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT		  0
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT	  10
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT	  20
> +
> +#define EDP_PSR2_SU_STATUS2					  _MMIO(0x6F918)
> +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK			  0x3FF
> +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT  0
> +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT  10
> +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT  20
> +
> +#define EDP_PSR2_SU_STATUS3					  _MMIO(0x6F91C)
> +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK			  0x3FF
> +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT  0
> +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT  10
> +

Couldn't we define it unified as:
+#define EDP_PSR2_SU_STATUS(frame)				_MMIO(0x6F914 + 4 * frame / 3)
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK			  0x3FF
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_SHIFT(frame)	(frame % 3 * 10)

>  /* VGA port control */
>  #define ADPA			_MMIO(0x61100)
>  #define PCH_ADPA                _MMIO(0xe1100)
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915/debugfs: Print how many blocks were sent in a selective update

[Souza, Jose]

On Thu, 2018-03-22 at 16:46 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:48PM -0700, José Roberto de Souza
> wrote:
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 40
> > ++++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h     | 17 ++++++++++++++++
> >  2 files changed, 56 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 3182e9a7cc5d..20985584cc0f 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2678,6 +2678,43 @@ static void psr_event_exit_sprintf(struct
> > seq_file *m, u32 val,
> >  		seq_puts(m, "\tPSR disabled\n");
> >  }
> >  
> > +static void psr2_su_blocks_sprintf(struct seq_file *m,
> > +				   struct drm_i915_private
> > *dev_priv)
> > +{
> > +	u32 val;
> > +	u16 su;
> > +
> > +	val = I915_READ(EDP_PSR2_SU_STATUS);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N: %d\n", su);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-1: %d\n", su);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-2: %d\n", su);
> > +
> > +	val = I915_READ(EDP_PSR2_SU_STATUS2);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-3: %d\n", su);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-4: %d\n", su);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-5: %d\n", su);
> > +
> > +	val = I915_READ(EDP_PSR2_SU_STATUS3);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-6: %d\n", su);
> > +	su = val >>
> > EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT;
> > +	su &= EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK;
> > +	seq_printf(m, "\tSU blocks in frame N-7: %d\n", su);
> > +}
> > +
> >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > >private);
> > @@ -2766,8 +2803,9 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  	if (dev_priv->psr.psr2_enabled) {
> >  		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
> >  
> > -		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> > +		seq_printf(m, "EDP_PSR2_STATUS: 0x%x [%s]\n",
> >  			   psr2, psr2_live_status(psr2));
> > +		psr2_su_blocks_sprintf(m, dev_priv);
> >  	}
> >  
> >  	if (dev_priv->psr.enabled) {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 45f7703a9ee6..18af3e8fd4b6 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3929,6 +3929,23 @@ enum {
> >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
> >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> >  
> > +#define EDP_PSR2_SU_STATUS					
> >   _MMIO(0x6F914)
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK			
> >   0x3FF
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_SHIFT		
> >   0
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_1_SHIFT	
> >   10
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_N_MINUS_2_SHIFT	
> >   20
> > +
> > +#define EDP_PSR2_SU_STATUS2					
> >   _MMIO(0x6F918)
> > +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_MASK			
> >   0x3FF
> > +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_3_SHIFT  
> > 0
> > +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_4_SHIFT  
> > 10
> > +#define  EDP_PSR2_SU_STATUS2_NUM_SU_BLOCKS_FRAME_N_MINUS_5_SHIFT  
> > 20
> > +
> > +#define EDP_PSR2_SU_STATUS3					
> >   _MMIO(0x6F91C)
> > +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_MASK			
> >   0x3FF
> > +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_6_SHIFT  
> > 0
> > +#define  EDP_PSR2_SU_STATUS3_NUM_SU_BLOCKS_FRAME_N_MINUS_7_SHIFT  
> > 10
> > +
> 
> Couldn't we define it unified as:
> +#define EDP_PSR2_SU_STATUS(frame)				_MM
> IO(0x6F914 + 4 * frame / 3)
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK			
>   0x3FF
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_SHIFT(frame)	
> (frame % 3 * 10)

Oh way better, thanks.
It also saved a lot of line when priting:

static void psr2_su_blocks_sprintf(struct seq_file *m,
				   struct drm_i915_private *dev_priv)
{
	unsigned i;

	for (i = 0; i < 8; i++) {
		u32 val = I915_READ(EDP_PSR2_SU_STATUS(i));
		val = val >>
EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_FRAME_SHIFT(i);
		val &= EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_MASK;
		seq_printf(m, "\tSU blocks in frame N-%d: %d\n", i,
val);
	}
}

> 
> >  /* VGA port control */
> >  #define ADPA			_MMIO(0x61100)
> >  #define PCH_ADPA                _MMIO(0xe1100)
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [01/12] drm: Add DP PSR2 sink enable bit

[Patchwork]

== Series Details ==

Series: series starting with [01/12] drm: Add DP PSR2 sink enable bit
URL   : https://patchwork.freedesktop.org/series/40521/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3c6c3ba8cde6 drm: Add DP PSR2 sink enable bit
67ced254aa32 drm: Add DP last received PSR SDP VSC register and bits
f7edbfc94e93 drm/i915/psr: Nuke aux frame sync
307f5aa1e5d2 drm/i915/psr: Tie PSR2 support to Y coordinate requirement
060b501ae2f1 drm/i915/psr/cnl: Enable Y-coordinate support in source
-:26: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#26: FILE: drivers/gpu/drm/i915/i915_reg.h:3895:
+#define   EDP_Y_COORDINATE_VALID	(1<<26) /* GLK and CNL+ */
                                 	  ^

-:27: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#27: FILE: drivers/gpu/drm/i915/i915_reg.h:3896:
+#define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
                                  	  ^

-:35: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#35: FILE: drivers/gpu/drm/i915/i915_reg.h:6903:
+#define  VSC_DATA_SEL_SOFTWARE_CONTROL	(1<<25) /* GLK and CNL+ */
                                       	  ^

total: 0 errors, 0 warnings, 3 checks, 43 lines checked
6659daf1e4d9 drm/i915/psr: Do not override PSR2 sink support
83a99661c27d drm/i915/psr: Use PSR2 macro for PSR2
ebf449282d26 drm/i915/psr: Cache sink synchronization latency
70148c4aea67 drm/i915/psr: Set DPCD PSR2 enable bit when needed
0f9a21c67357 drm/i915/debugfs: Print sink PSR state and debug info
5fda9554238b drm/i915/debugfs: Print information about what caused a PSR exit
ff1a90ddf554 drm/i915/debugfs: Print how many blocks were sent in a selective update

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [01/12] drm: Add DP PSR2 sink enable bit

[Patchwork]

== Series Details ==

Series: series starting with [01/12] drm: Add DP PSR2 sink enable bit
URL   : https://patchwork.freedesktop.org/series/40521/
State : failure

== Summary ==

Series 40521v1 series starting with [01/12] drm: Add DP PSR2 sink enable bit
https://patchwork.freedesktop.org/api/1.0/series/40521/revisions/1/mbox/

---- Possible new issues:

Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> INCOMPLETE (fi-cnl-y3)

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:433s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:387s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:298s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:514s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:518s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:520s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:508s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:412s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:512s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:519s
fi-cnl-y3        total:284  pass:258  dwarn:0   dfail:0   fail:0   skip:25 
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:427s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:316s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:536s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:405s
fi-ilk-650       total:285  pass:224  dwarn:0   dfail:0   fail:1   skip:60  time:422s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:435s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:478s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:512s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:666s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:443s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:529s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:428s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:600s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:405s
fi-cfl-s3 failed to connect after reboot

9e3f06af248708774c765efd55bf8e883f24fbfd drm-tip: 2018y-03m-22d-21h-17m-42s UTC integration manifest
ff1a90ddf554 drm/i915/debugfs: Print how many blocks were sent in a selective update
5fda9554238b drm/i915/debugfs: Print information about what caused a PSR exit
0f9a21c67357 drm/i915/debugfs: Print sink PSR state and debug info
70148c4aea67 drm/i915/psr: Set DPCD PSR2 enable bit when needed
ebf449282d26 drm/i915/psr: Cache sink synchronization latency
83a99661c27d drm/i915/psr: Use PSR2 macro for PSR2
6659daf1e4d9 drm/i915/psr: Do not override PSR2 sink support
060b501ae2f1 drm/i915/psr/cnl: Enable Y-coordinate support in source
307f5aa1e5d2 drm/i915/psr: Tie PSR2 support to Y coordinate requirement
f7edbfc94e93 drm/i915/psr: Nuke aux frame sync
67ced254aa32 drm: Add DP last received PSR SDP VSC register and bits
3c6c3ba8cde6 drm: Add DP PSR2 sink enable bit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8461/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm: Add DP PSR2 sink enable bit

[Rodrigo Vivi]

On Thu, Mar 22, 2018 at 02:48:37PM -0700, José Roberto de Souza wrote:
> To comply with eDP1.4a this bit should be set when enabling PSR2.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

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

> ---
>  include/drm/drm_dp_helper.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 62903bae0221..0bac0c7d0dec 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -478,6 +478,7 @@
>  # define DP_PSR_FRAME_CAPTURE		    (1 << 3)
>  # define DP_PSR_SELECTIVE_UPDATE	    (1 << 4)
>  # define DP_PSR_IRQ_HPD_WITH_CRC_ERRORS     (1 << 5)
> +# define DP_PSR_ENABLE_PSR2		    (1 << 6) /* eDP 1.4a */
>  
>  #define DP_ADAPTER_CTRL			    0x1a0
>  # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE   (1 << 0)
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx