Re: [Intel-gfx] [PATCH 6/6] drm/i915/psr: Fix ALPM cap check for PSR2

[Dhinakaran Pandiyan]

On Tue, 2018-05-22 at 07:37 -0700, Tarun Vyas wrote:
> On Fri, May 11, 2018 at 12:51:45PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > While touching the code around this, I noticed that absence of ALPM
> > capability does not stop us from enabling PSR2. But, the spec
> > unambiguously states that ALPM is required for PSR2 and so does
> > this
> > commit that introduced this code
> > 
> > drm/i915/psr: enable ALPM for psr2
> > 
> > As per edp1.4 spec , alpm is required for psr2 operation as
> > it's
> > used for all psr2  main link power down management and alpm
> > enable
> > bit must be set for psr2 operation.
> > 
> Since, the code introduced by "drm/i915/psr: enable ALPM for psr2"
> enables PSR2 even if ALPM isn't supported, can we add the "Fixes" tag
> here ?

I thought about it. I don't think PSR2 was enabled upstream by default,
so we should be good without Fixes. And I didn't investigate if the
original commit missed the ALPM check or if it was mangled later.

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

Re: [Intel-gfx] [PATCH 6/6] drm/i915/psr: Fix ALPM cap check for PSR2

[Tarun Vyas]

On Fri, May 11, 2018 at 12:51:45PM -0700, Dhinakaran Pandiyan wrote:
> While touching the code around this, I noticed that absence of ALPM
> capability does not stop us from enabling PSR2. But, the spec
> unambiguously states that ALPM is required for PSR2 and so does this
> commit that introduced this code
> 
> drm/i915/psr: enable ALPM for psr2
> 
> As per edp1.4 spec , alpm is required for psr2 operation as it's
> used for all psr2  main link power down management and alpm enable
> bit must be set for psr2 operation.
>
Since, the code introduced by "drm/i915/psr: enable ALPM for psr2" enables PSR2 
even if ALPM isn't supported, can we add the "Fixes" tag here ? Rest looks good.

Reviewed-by: Tarun Vyas  
> Cc: Jose Roberto de Souza 
> Cc: Vathsala Nagaraju 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index b4a4f5d3a2bb..92abf61e234c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -254,6 +254,10 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  
>   if (INTEL_GEN(dev_priv) >= 9 &&
>   (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> + bool y_req = intel_dp->psr_dpcd[1] &
> +  DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> + bool alpm = intel_dp_get_alpm_status(intel_dp);
> +
>   /*
>* All panels that supports PSR version 03h (PSR2 +
>* Y-coordinate) can handle Y-coordinates in VSC but we are
> @@ -265,16 +269,13 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>* Y-coordinate requirement panels we would need to enable
>* GTC first.
>*/
> - dev_priv->psr.sink_psr2_support =
> - intel_dp->psr_dpcd[1] & 
> DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> + dev_priv->psr.sink_psr2_support = y_req && alpm;
>   DRM_DEBUG_KMS("PSR2 %ssupported\n",
> dev_priv->psr.sink_psr2_support ? "" : "not ");
>  
>   if (dev_priv->psr.sink_psr2_support) {
>   dev_priv->psr.colorimetry_support =
>   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);
>   }
> @@ -386,13 +387,12 @@ static void hsw_psr_enable_sink(struct intel_dp 
> *intel_dp)
>   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(_dp->aux,
> - DP_RECEIVER_ALPM_CONFIG,
> - DP_ALPM_ENABLE);
> -
> - if (dev_priv->psr.psr2_enabled)
> + if (dev_priv->psr.psr2_enabled) {
> + drm_dp_dpcd_writeb(_dp->aux, DP_RECEIVER_ALPM_CONFIG,
> +DP_ALPM_ENABLE);
>   dpcd_val |= DP_PSR_ENABLE_PSR2;
> + }
> +
>   if (dev_priv->psr.link_standby)
>   dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
>   drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, dpcd_val);
> -- 
> 2.14.1
> 
> ___
> 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: [Intel-gfx] [PATCH 6/6] drm/i915/psr: Fix ALPM cap check for PSR2

[Nagaraju, Vathsala]

On 5/12/2018 1:21 AM, Dhinakaran Pandiyan wrote:

While touching the code around this, I noticed that absence of ALPM
capability does not stop us from enabling PSR2. But, the spec
unambiguously states that ALPM is required for PSR2 and so does this
commit that introduced this code

drm/i915/psr: enable ALPM for psr2

 As per edp1.4 spec , alpm is required for psr2 operation as it's
 used for all psr2  main link power down management and alpm enable
 bit must be set for psr2 operation.

Cc: Jose Roberto de Souza 
Cc: Vathsala Nagaraju 
Signed-off-by: Dhinakaran Pandiyan 
---
  drivers/gpu/drm/i915/intel_psr.c | 20 ++--
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b4a4f5d3a2bb..92abf61e234c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -254,6 +254,10 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
  
  	if (INTEL_GEN(dev_priv) >= 9 &&

(intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
+   bool y_req = intel_dp->psr_dpcd[1] &
+DP_PSR2_SU_Y_COORDINATE_REQUIRED;
+   bool alpm = intel_dp_get_alpm_status(intel_dp);
+
/*
 * All panels that supports PSR version 03h (PSR2 +
 * Y-coordinate) can handle Y-coordinates in VSC but we are
@@ -265,16 +269,13 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 * Y-coordinate requirement panels we would need to enable
 * GTC first.
 */
-   dev_priv->psr.sink_psr2_support =
-   intel_dp->psr_dpcd[1] & 
DP_PSR2_SU_Y_COORDINATE_REQUIRED;
+   dev_priv->psr.sink_psr2_support = y_req && alpm;
DRM_DEBUG_KMS("PSR2 %ssupported\n",
  dev_priv->psr.sink_psr2_support ? "" : "not ");
  
  		if (dev_priv->psr.sink_psr2_support) {

dev_priv->psr.colorimetry_support =
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);
}
@@ -386,13 +387,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
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(_dp->aux,
-   DP_RECEIVER_ALPM_CONFIG,
-   DP_ALPM_ENABLE);
-
-   if (dev_priv->psr.psr2_enabled)
+   if (dev_priv->psr.psr2_enabled) {
+   drm_dp_dpcd_writeb(_dp->aux, DP_RECEIVER_ALPM_CONFIG,
+  DP_ALPM_ENABLE);
dpcd_val |= DP_PSR_ENABLE_PSR2;
+   }

ALPM is needed for fast wake.
Reviewed-by: Vathsala Nagaraju 

+
if (dev_priv->psr.link_standby)
dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, dpcd_val);


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

[Intel-gfx] [PATCH 6/6] drm/i915/psr: Fix ALPM cap check for PSR2

[Dhinakaran Pandiyan]

While touching the code around this, I noticed that absence of ALPM
capability does not stop us from enabling PSR2. But, the spec
unambiguously states that ALPM is required for PSR2 and so does this
commit that introduced this code

drm/i915/psr: enable ALPM for psr2

As per edp1.4 spec , alpm is required for psr2 operation as it's
used for all psr2  main link power down management and alpm enable
bit must be set for psr2 operation.

Cc: Jose Roberto de Souza 
Cc: Vathsala Nagaraju 
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/i915/intel_psr.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b4a4f5d3a2bb..92abf61e234c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -254,6 +254,10 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 
if (INTEL_GEN(dev_priv) >= 9 &&
(intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
+   bool y_req = intel_dp->psr_dpcd[1] &
+DP_PSR2_SU_Y_COORDINATE_REQUIRED;
+   bool alpm = intel_dp_get_alpm_status(intel_dp);
+
/*
 * All panels that supports PSR version 03h (PSR2 +
 * Y-coordinate) can handle Y-coordinates in VSC but we are
@@ -265,16 +269,13 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 * Y-coordinate requirement panels we would need to enable
 * GTC first.
 */
-   dev_priv->psr.sink_psr2_support =
-   intel_dp->psr_dpcd[1] & 
DP_PSR2_SU_Y_COORDINATE_REQUIRED;
+   dev_priv->psr.sink_psr2_support = y_req && alpm;
DRM_DEBUG_KMS("PSR2 %ssupported\n",
  dev_priv->psr.sink_psr2_support ? "" : "not ");
 
if (dev_priv->psr.sink_psr2_support) {
dev_priv->psr.colorimetry_support =
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);
}
@@ -386,13 +387,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
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(_dp->aux,
-   DP_RECEIVER_ALPM_CONFIG,
-   DP_ALPM_ENABLE);
-
-   if (dev_priv->psr.psr2_enabled)
+   if (dev_priv->psr.psr2_enabled) {
+   drm_dp_dpcd_writeb(_dp->aux, DP_RECEIVER_ALPM_CONFIG,
+  DP_ALPM_ENABLE);
dpcd_val |= DP_PSR_ENABLE_PSR2;
+   }
+
if (dev_priv->psr.link_standby)
dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, dpcd_val);
-- 
2.14.1

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