Re: [PATCH] drm/i915/skl: Enabling PSR2 SU with frame sync

[sonika <sonika.jindal@intel.com>]

On Saturday 21 March 2015 02:50 AM, Vivi, Rodrigo wrote:
> On Fri, 2015-03-20 at 11:27 +0530, Sonika Jindal wrote:
>> We make use of HW tracking for Selective update region and enable frame sync on
>> sink. We use hardware's hardcoded data values for frame sync and GTC.
> Before enabling HW tracking for PSR2 I'd like to know if all known bad
> cases of bad HW tracking verification now works well.
> Shouldn't we also remove all masks to be sure?
>
> But yeah, or that or we need to create an interface probably using ioctl
> to userspace select the partial area to be updated.
>
> I prefer HW tracking but I just want to be sure we aren't breaking the
> world. So I would start checking KDE behaviour, login screens, fbcon,
> xterm, etc.
>
> But also this patch doesnt' change to use HW tracking, but anyway we
> start to add the support what is cool already.
Yes, since with this patch we are not interrupting the PSR functionality 
and with PSR's SW tracking being there, this will not really affect.

For SW tracking with PSR2, we don't have any clear way as per bspec to 
supply rectangles.
Besides, its a better idea to just let it have it this way until we can 
really try it out on PSR2 panels.

So, HW tracking for PSR2 as of now which should be fine because for any 
screen update we anyways will be doing PSR exits.
For some small updates, if we miss I am hoping that PSR2's HW tracking 
will take care :)

Since, we don't still have any panel which support PSR2, I have not 
given it a try yet.

>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h  |   14 ++++++++++++++
>>   drivers/gpu/drm/i915/intel_dp.c  |   16 ++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |    2 ++
>>   drivers/gpu/drm/i915/intel_psr.c |   30 +++++++++++++++++++++++++++++-
>>   4 files changed, 61 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 5b84ee6..7e4f6f0 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2745,6 +2745,20 @@ enum skl_disp_power_wells {
>>   #define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
>>   #define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
>>   
>> +#define EDP_PSR2_CTL			0x6f900
>> +#define   EDP_PSR2_ENABLE		(1<<31)
>> +#define   EDP_SU_TRACK_ENABLE		(1<<30)
>> +#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)
>> +#define   EDP_PSR2_TP2_TIME_100		(1<<8)
>> +#define   EDP_PSR2_TP2_TIME_250		(2<<8)
>> +#define   EDP_PSR2_TP2_TIME_50		(3<<8)
>> +#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
>> +
>>   /* VGA port control */
>>   #define ADPA			0x61100
>>   #define PCH_ADPA                0xe1100
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3967af1..dedd8ad 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3786,6 +3786,22 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   			dev_priv->psr.sink_support = true;
>>   			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>   		}
>> +
>> +		if (INTEL_INFO(dev)->gen >= 9 &&
>> +			(intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
>> +			uint8_t frame_sync_cap;
>> +
>> +			dev_priv->psr.sink_support = true;
>> +			intel_dp_dpcd_read_wake(&intel_dp->aux,
>> +					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> Shouldn't we verify if it eDP 1.4 before reading this register that
> didn't existed previously?
Not required. This register (0070h PSR caps) always existed with 0 and 1 
values. Value 2 (DP_PSR2_IS_SUPPORTED) was introduced in edp1.4.
>> +					&frame_sync_cap, 1);
>> +			/* PSR2 needs frame sync as well */
> But anyway, this frame sync cap was created and added for PSR2 so isn't
> DP_PSR2_IS_SUPPORTED already a supper seed and enough?
No, frame sync is not just for PSR2. It is there for general AV sync 
"during periods of temporary video stream inactivity on main-link".
Other usages might be added in future versions.
> In the case they are really independent we could track with different
> variable and use that when writing DP_AUX_FRAME_SYNC_ENABLE.
>
Hmm, we could do that too.
>> +			if (frame_sync_cap) {
>> +				DRM_DEBUG_KMS("PSR2 supported on sink");
>> +				intel_dp->psr2_support = true;
>> +			} else
>> +				intel_dp->psr2_support = false;
>> +		}
>>   	}
>>   
>>   	/* Training Pattern 3 support, both source and sink */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 8bb18e5..ed1b0a5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -663,6 +663,8 @@ struct intel_dp {
>>   	struct intel_dp_mst_encoder *mst_encoders[I915_MAX_PIPES];
>>   	struct drm_dp_mst_topology_mgr mst_mgr;
>>   
>> +	bool psr2_support;
> Please move this to psr structure.
>
Makes sense.
>> +
>>   	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>>   	/*
>>   	 * This function returns the value we have to program the AUX_CTL
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index b9f40c2..99dbc73 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -117,6 +117,19 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
>>   	I915_WRITE(VLV_VSCSDP(pipe), val);
>>   }
>>   
>> +static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
>> +{
>> +	struct edp_vsc_psr psr_vsc;
>> +
>> +	/* 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;
>> +	psr_vsc.sdp_header.HB1 = 0x7;
>> +	psr_vsc.sdp_header.HB2 = 0x3;
>> +	psr_vsc.sdp_header.HB3 = 0xb;
>> +	intel_psr_write_vsc(intel_dp, &psr_vsc);
>> +}
>> +
>>   static void hsw_psr_setup_vsc(struct intel_dp *intel_dp)
>>   {
>>   	struct edp_vsc_psr psr_vsc;
>> @@ -165,6 +178,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>>   		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>>   				   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>>   
>> +	/* Enable AUX frame sync at sink */
>> +	if (intel_dp->psr2_support)
>> +		drm_dp_dpcd_writeb(&intel_dp->aux,
>> +				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
>> +				DP_AUX_FRAME_SYNC_ENABLE);
>> +
>>   	aux_data_reg = (INTEL_INFO(dev)->gen >= 9) ?
>>   				DPA_AUX_CH_DATA1 : EDP_PSR_AUX_DATA1(dev);
>>   	aux_ctl_reg = (INTEL_INFO(dev)->gen >= 9) ?
>> @@ -183,8 +202,10 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>>   		val |= DP_AUX_CH_CTL_TIME_OUT_1600us;
>>   		val &= ~DP_AUX_CH_CTL_MESSAGE_SIZE_MASK;
>>   		val |= (sizeof(aux_msg) << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
>> -		/* Use hardcoded data values for PSR */
>> +		/* Use hardcoded data values for PSR, frame sync and GTC */
>>   		val &= ~DP_AUX_CH_CTL_PSR_DATA_AUX_REG_SKL;
>> +		val &= ~DP_AUX_CH_CTL_FS_DATA_AUX_REG_SKL;
>> +		val &= ~DP_AUX_CH_CTL_GTC_DATA_AUX_REG_SKL;
>>   		I915_WRITE(aux_ctl_reg, val);
>>   	} else {
>>   		I915_WRITE(aux_ctl_reg,
>> @@ -255,6 +276,10 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>>   		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>>   		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
>>   		   EDP_PSR_ENABLE);
>> +
>> +	if (intel_dp->psr2_support)
>> +		I915_WRITE(EDP_PSR2_CTL, EDP_PSR2_ENABLE |
>> +				EDP_SU_TRACK_ENABLE | EDP_PSR2_TP2_TIME_100);
>>   }
>>   
>>   static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
>> @@ -364,6 +389,9 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>>   	if (HAS_DDI(dev)) {
>>   		hsw_psr_setup_vsc(intel_dp);
>>   
>> +		if (intel_dp->psr2_support)
>> +			skl_psr_setup_su_vsc(intel_dp);
>> +
>>   		/* Avoid continuous PSR exit by masking memup and hpd */
>>   		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>>   			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);

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