Re: [PATCH 7/9] drm/i915: Enable/Disable PSR on HSW

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

On Wed, 30 Jan 2013, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> From: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Added eDP PSR enable functionality. This includes setting the PSR
> configuration over AUX, sending SDP VSC DIP over the eDP PIPE config,
> enabling PSR in the sink via DPCD register and finally enabling PSR on
> the host. PSR works only in LPSP mode, so put the PIPE_DDI in DDIA
> always on
>
> This patch is heavily based on initial PSR code by Sateesh Kavuri but is
> quite different in implementation. Makes use of VBT parsed data and also
> the code has been cleaned up.
>
> Credits-by: Sateesh Kavuri <sateesh.kavuri@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> v2: fix getting base.crtc from intel_dp and fix DDI_EDP_INPUT_A_ON entry
> v3: Add eDP PSR registers here only when they are really used and use
>     HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder)

Some nitpicks and bikeshedding below, but regardless,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  24 ++++++
>  drivers/gpu/drm/i915/intel_ddi.c |   6 +-
>  drivers/gpu/drm/i915/intel_dp.c  | 172 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   3 +
>  4 files changed, 204 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 10732dc..6f87d5b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1563,6 +1563,30 @@
>  #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
>  #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
>  
> +/* HSW eDP PSR registers */
> +#define EDP_PSR_CTL				0x64800
> +#define   EDP_PSR_ENABLE			(1<<31)
> +#define   EDP_PSR_LINK_DISABLE			(0<<27)
> +#define   EDP_PSR_LINK_STANDBY			(1<<27)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK	(3<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES	(0<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES	(1<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES	(2<<25)
> +#define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES	(3<<25)
> +#define   EDP_PSR_MAX_SLEEP_TIME_SHIFT		20
> +#define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
> +#define   EDP_PSR_TP1_TP2_SEL			(0<<11)
> +#define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> +#define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> +#define   EDP_PSR_TP2_TP3_TIME_0us		(3<<8)
> +#define   EDP_PSR_TP1_TIME_500us		(0<<4)
> +#define   EDP_PSR_TP1_TIME_100us		(1<<4)
> +#define   EDP_PSR_TP1_TIME_2500us		(2<<4)
> +#define   EDP_PSR_TP1_TIME_0us			(3<<4)
> +#define   EDP_PSR_IDLE_FRAME_SHIFT		0
> +
>  #define EDP_PSR_AUX_CTL			0x64810
>  #define EDP_PSR_AUX_DATA1		0x64814
>  #define EDP_PSR_AUX_DATA2		0x64818
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2e904a5..feb7a4a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -985,9 +985,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
>  		temp |= TRANS_DDI_PHSYNC;
>  
>  	if (cpu_transcoder == TRANSCODER_EDP) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  		switch (pipe) {
>  		case PIPE_A:
> -			temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
> +			if (intel_dp->psr_dpcd[0] & 0x1)

s/0x1/DP_PSR_IS_SUPPORTED/

Ditto elsewhere below.

> +				temp |= TRANS_DDI_EDP_INPUT_A_ON;
> +			else
> +				temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
>  			break;
>  		case PIPE_B:
>  			temp |= TRANS_DDI_EDP_INPUT_B_ONOFF;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fe83e05..d136890 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -83,6 +83,13 @@ static struct drm_device *intel_dp_to_dev(struct intel_dp *intel_dp)
>  	return intel_dig_port->base.base.dev;
>  }
>  
> +static struct drm_crtc *intel_dp_to_crtc(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +
> +	return intel_dig_port->base.base.crtc;
> +}
> +
>  static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
>  {
>  	return enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> @@ -1489,6 +1496,171 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>  	intel_dp->psr_setup = 1;
>  }
>  
> +static bool
> +intel_edp_is_psr_enabled(struct intel_dp* intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	return (I915_READ(EDP_PSR_CTL) & (1<<31)) ? true : false;

	return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;

> +}
> +
> +
> +static void
> +intel_edp_psr_enable_src(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t max_sleep_time = 0x1f;
> +	uint32_t val = 0x0;
> +
> +	/* Use VBT values which are parsed in
> +	 * dev_priv->idle_frames,
> +	 * but the BIOS initializes this to zero today
> +	 * so hardcode
> +	 */
> +	uint32_t idle_frames = 6;
> +
> +	if (intel_dp->psr_dpcd[1] & 0x1) {
> +		/* No link training on PSR Exit required */
> +		val |= EDP_PSR_TP2_TP3_TIME_0us;
> +		val |= EDP_PSR_TP1_TIME_0us;
> +		val |= EDP_PSR_SKIP_AUX_EXIT;
> +	} else {
> +		/* Use these Values from VBT
> +		 * Case values are timings for HSW as of now
> +		 * in multiple of 100us
> +		 */
> +		switch(dev_priv->vbt.psr_wakeup_tp1) {
> +			case 1:
> +				val |= EDP_PSR_TP1_TIME_100us;
> +				break;
> +			case 5:
> +				val |= EDP_PSR_TP1_TIME_500us;
> +				break;
> +			case 25:
> +				val |= EDP_PSR_TP1_TIME_2500us;
> +				break;
> +			default:
> +				val |= EDP_PSR_TP1_TIME_500us;
> +				break;
> +		};
> +		switch(dev_priv->vbt.psr_wakeup_tp2_tp3) {
> +			case 1:
> +				val |= EDP_PSR_TP2_TP3_TIME_100us;
> +				break;
> +			case 5:
> +				val |= EDP_PSR_TP2_TP3_TIME_500us;
> +				break;
> +			case 25:
> +				val |= EDP_PSR_TP2_TP3_TIME_2500us;
> +				break;
> +			default:
> +				val |= EDP_PSR_TP2_TP3_TIME_500us;
> +				break;
> +		};
> +	}
> +
> +	/* Disable main link. Anyway in HSW steppings today
> +	 * link standby does not work
> +	 *
> +	 * Later used VBT info (already parsed and available)
> +	 * while supporting standby we need to program
> +	 * val |= EDP_PSR_MIN_LINK_ENTRY_TIME_X_LINES based on VBT
> +	 */
> +	val = (val & ~EDP_PSR_LINK_STANDBY) |
> +		(max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT) |
> +		(idle_frames << EDP_PSR_IDLE_FRAME_SHIFT) |
> +		EDP_PSR_ENABLE;
> +
> +	I915_WRITE(EDP_PSR_CTL, val);
> +}
> +
> +void intel_edp_enable_psr(struct intel_dp* intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp_to_crtc(intel_dp));
> +	struct edp_vsc_psr psr_vsc;
> +	uint32_t reg = HSW_TVIDEO_DIP_AVI_DATA(intel_crtc->cpu_transcoder);
> +	uint32_t *vsc_data = (uint32_t *) &psr_vsc;
> +	int i = 0, vsc_len = sizeof(struct edp_vsc_psr);
> +
> +	if (!is_edp_psr(intel_dp))
> +		return;
> +
> +	/* setup AUX registers in case returned from pm states */
> +	intel_edp_psr_setup(intel_dp);
> +
> +	/* Check if PSR is already enabled */
> +	if (!intel_edp_is_psr_enabled(intel_dp)) {

I'd rather make this return early if PSR is enabled, and reduce indent
on the lines below.

> +		/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> +		memset(&psr_vsc, 0, sizeof(psr_vsc));
> +		psr_vsc.sdp_header.id = 0;
> +		psr_vsc.sdp_header.type = 0x7;
> +		psr_vsc.sdp_header.revision = 0x2;
> +		psr_vsc.sdp_header.valid_payload_bytes = 0x8;
> +
> +		/* As per eDP spec, wait for vblank to send SDP VSC packet */
> +		intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +		/* Load the VSC DIP packet */
> +		for(i = 0; i < vsc_len; i += 4)

Just use sizeof here too instead of a temp variable for vsc_len.

> +			I915_WRITE((reg + i), vsc_data[i]);

Unnecessary braces around reg + i.

I guess we can be sure sizeof(struct edp_vsc_psr) % 4 == 0 always.

> +
> +#if 0
> +		/* TBD:
> +		 * We might not have to do explicitely as hardware will take care of this */
> +		/* Enable the DIP register */
> +		val = I915_READ(VIDEO_DIP_CTL_EDP);
> +		I915_WRITE(VIDEO_DIP_CTL_EDP, val | VIDEOP_DIP_VSC);
> +#endif
> +		/* Enable PSR in sink by setting bit 0 in DPCD config reg
> +		 * along with the transmitter state during PSR active
> +		 * Transmitter state later can be ready from VBT. As of now
> +		 * program the full link down
> +		 *
> +		 */
> +		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +						DP_PSR_ENABLE &
> +						~DP_PSR_MAIN_LINK_ACTIVE);
> +
> +		/* Enable PSR on the host */
> +		intel_edp_psr_enable_src(intel_dp);
> +	}
> +}
> +
> +void intel_edp_disable_psr(struct intel_dp* intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp_to_crtc(intel_dp));
> +	uint32_t val;
> +	if (!intel_edp_is_psr_enabled(intel_dp))
> +		return;
> +
> +	val = I915_READ(EDP_PSR_CTL);
> +	I915_WRITE(EDP_PSR_CTL, (val & ~EDP_PSR_ENABLE));

Unnecessary braces.

> +
> +	/* Wait till PSR is idle */
> +	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) & EDP_PSR_STATUS_MASK) == 0, 2000, 10))
> +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> +	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +#if 0
> +	/* TBD:
> +	 * Following is not yet confirmed from H/W team.
> +	 * As per last discussion we do not need to disable
> +	 * VSC DIP explicitely. Just maintaining the code in
> +	 * case we have to do this later at some point
> +	 */
> +
> +	/* Disable VSC DIP */
> +	val = I915_READ(VIDEO_DIP_CTL_EDP);
> +	I915_WRITE(VIDEO_DIP_CTL_EDP, val & ~VIDEOP_DIP_VSC);
> +#endif
> +}
> +
>  static void intel_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3fa2dd0..2f48e5c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -692,4 +692,7 @@ extern bool
>  intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>  extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
>  
> +extern void intel_edp_enable_psr(struct intel_dp* intel_dp);
> +extern void intel_edp_disable_psr(struct intel_dp* intel_dp);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.7.11.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel