Re: [PATCH 1/2] drm/i915/psr: Introduce HAS_VLV_PSR

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 11 Jul 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> This old PSR implementation died on gen8LP and had many
> limitations such:
> - no functional HW tracking
> - required link standby with single frame updates
>
> So, let's group them in a easier way to filter it now on.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 5 ++++-
>  drivers/gpu/drm/i915/i915_pci.c  | 4 ++--
>  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
>  drivers/gpu/drm/i915/intel_psr.c | 7 +++----
>  4 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 81cd21ecfa7d..d0a49307c6fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -769,6 +769,7 @@ struct intel_csr {
>  	func(has_pipe_cxsr); \
>  	func(has_pooled_eu); \
>  	func(has_psr); \
> +	func(has_vlv_psr); \
>  	func(has_rc6); \
>  	func(has_rc6p); \
>  	func(has_resource_streamer); \
> @@ -2983,7 +2984,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>  
>  #define HAS_DDI(dev_priv)		 ((dev_priv)->info.has_ddi)
>  #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) ((dev_priv)->info.has_fpga_dbg)
> -#define HAS_PSR(dev_priv)		 ((dev_priv)->info.has_psr)
> +#define HAS_PSR(dev_priv)		 ((dev_priv)->info.has_psr || \
> +					  (dev_priv)->info.has_vlv_psr)
> +#define HAS_VLV_PSR(dev_priv)		 ((dev_priv)->info.has_vlv_psr)

I'm not convinced adding a intel info field for this is warranted. How
about just:

#define HAS_VLV_PSR(dev_priv) (HAS_PSR(dev_priv) && \
	(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)))

Though I'm not convinced about the HS_VLV_PSR name either!

BR,
Jani.


>  #define HAS_RC6(dev_priv)		 ((dev_priv)->info.has_rc6)
>  #define HAS_RC6p(dev_priv)		 ((dev_priv)->info.has_rc6p)
>  
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index a1e6b696bcfa..e0e972210090 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -274,7 +274,7 @@ static const struct intel_device_info intel_valleyview_info = {
>  	.gen = 7,
>  	.is_lp = 1,
>  	.num_pipes = 2,
> -	.has_psr = 1,
> +	.has_vlv_psr = 1,
>  	.has_runtime_pm = 1,
>  	.has_rc6 = 1,
>  	.has_gmbus_irq = 1,
> @@ -334,7 +334,7 @@ static const struct intel_device_info intel_cherryview_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>  	.platform = INTEL_CHERRYVIEW,
>  	.has_64bit_reloc = 1,
> -	.has_psr = 1,
> +	.has_vlv_psr = 1,
>  	.has_runtime_pm = 1,
>  	.has_resource_streamer = 1,
>  	.has_rc6 = 1,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2d42d09428c9..3f2191edc2c9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2653,7 +2653,7 @@ static void intel_disable_dp(struct intel_encoder *encoder,
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
> -	if (HAS_PSR(dev_priv) && !HAS_DDI(dev_priv))
> +	if (HAS_VLV_PSR(dev_priv))
>  		intel_psr_disable(intel_dp);
>  
>  	/* Make sure the panel is off before trying to change the mode. But also
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 559f1ab42bfc..461c7e4f6ecc 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -401,8 +401,7 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> -	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> -	    !dev_priv->psr.link_standby) {
> +	if (HAS_VLV_PSR(dev_priv) && !dev_priv->psr.link_standby) {
>  		DRM_ERROR("PSR condition failed: Link off requested but not supported on this platform\n");
>  		return false;
>  	}
> @@ -827,7 +826,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
>  	 * Single frame update is already supported on BDW+ but it requires
>  	 * many W/A and it isn't really needed.
>  	 */
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> +	if (!HAS_VLV_PSR(dev_priv))
>  		return;
>  
>  	mutex_lock(&dev_priv->psr.lock);
> @@ -949,7 +948,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		/* HSW and BDW require workarounds that we don't implement. */
>  		dev_priv->psr.link_standby = false;
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	else if (HAS_VLV_PSR(dev_priv))
>  		/* On VLV and CHV only standby mode is supported. */
>  		dev_priv->psr.link_standby = true;
>  	else

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx