Re: [PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Mar 14, 2018 at 03:36:13PM -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 | 55 ++++++++++++++++++++--------------------
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8a584273f897..d4bc8d18f56c 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 9811f5f0bc75..62d97d5576d1 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;
>  
> @@ -137,22 +137,38 @@ 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;
> +		uint8_t frame_sync_cap, y_coord_req;
>  
>  		dev_priv->psr.sink_support = true;
> +
> +		/* PSR2 needs frame sync to do selective updates */
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
>  				      &frame_sync_cap) != 1)
>  			frame_sync_cap = 0;
>  		frame_sync_cap = (frame_sync_cap & DP_AUX_FRAME_SYNC_CAP);
> -		/* PSR2 needs frame sync as well */
> -		dev_priv->psr.psr2_support = frame_sync_cap;
> -		DRM_DEBUG_KMS("PSR2 %s on sink",
> -			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> +
> +		/*
> +		 * FIXME: Enable PSR2 only for y-coordinate PSR2 panels
> +		 * After GTC implementation, remove this restriction.

I believe we should remove the FIXME and just comment out that non y-coordinate
would need GTC that we currently don't implement.

later if we change our minds and decided to add that support we revisit here
but without a FIXME poluting code for something we don't plan to support for now.

> +		 *
> +		 * All panels that supports PSR version 3 that is

PSRv3? is there really something formal about that?

> +		 * PSR2 + Y-coordinate support 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 even without a valid aux frame sync.
> +		 */
> +		y_coord_req = intel_dp_get_y_coord_required(intel_dp);
> +
> +		dev_priv->psr.psr2_support = frame_sync_cap && y_coord_req;
> +		if (dev_priv->psr.psr2_support)
> +			DRM_DEBUG_KMS("PSR2 supported on sink\n");
> +		else
> +			DRM_DEBUG_KMS("PSR2 not supported on sink"
> +				      "(frame sync: %d Y-coord required: %d)\n",
> +				      frame_sync_cap, y_coord_req);
>  
>  		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 =
> @@ -198,16 +214,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 */
> @@ -478,15 +490,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;
>  }
>  
> @@ -586,14 +589,12 @@ 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);
>  
>  	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