Re: [PATCH 03/12] drm/i915/psr: Nuke aux frame sync

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-03-22 at 15:57 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:39PM -0700, José Roberto de Souza wrote:
> > Without GTC enabled hardware is sending dummy aux frame sync value

Curious, is this something you found by testing?

> > that is not useful to sink do selective update, that is why it also
> > require that sink supports and requires the y-coordinate.
> > 
> > So removing everything related to aux frame sync, if GTC is enabled
> > we can bring this back.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> (but I would like to give a time for Vathsala to comment on this)
> 

Reading the spec again,

No aux frame sync implies that the driver does not support selective
update, irrespective of whether we *support y-coordinate or not*.

Section 7.1 eDP v1.4b
"AUX_FRAME_SYNC was added to eDP v1.4 for streamlining the RFB
management of a Sink device during PSR2 operation. AUX_FRAME_SYNC is
required a for PSR2 when using the Selective Update feature (which is
any time that the SU_VALID bit p is set to 1 in the PSR2 VSC SDP).
Neither GTC nor AUX_FRAME_SYNC is required for PSR2 when using only the
Single-frame Update feature (which means that the SU_VALID bit is always
cleared to 0 in the PSR2 VSC SDP)."

Sending y-coordinate only allows for a lower precision aux frame sync
functionality, doesn't invalidate the need for aux frame sync.

"When the Source device includes the optional Y-coordinate in the SDP
for PSR2 Operation, as described in Table 6-12, the Sink device can
implement a lower-precision GTC Slave  function, as described in Table
7-1."

Aside from the still slightly misleading commit message, this in my
opinion is a good step. Let's make it obvious and clear that only full
frame updates are currently supported. We can then start working on
what's required for selective update and do it properly.


I'd like you to include relevant portions of the above text in the
commit message if you agree with the interpretation.

-DK



> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/intel_psr.c | 23 +----------------------
> >  2 files changed, 1 insertion(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c9c3b2ba6a86..7fe00509e51a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -602,7 +602,6 @@ struct i915_psr {
> >  	struct delayed_work work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> > -	bool aux_frame_sync;
> >  	bool link_standby;
> >  	bool y_cord_support;
> >  	bool colorimetry_support;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index b8e083e10029..d46320a451d9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -137,16 +137,9 @@ 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;
> >  
> >  		dev_priv->psr.sink_support = true;
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -				      &frame_sync_cap) != 1)
> > -			frame_sync_cap = 0;
> > -		dev_priv->psr.aux_frame_sync = frame_sync_cap & DP_AUX_FRAME_SYNC_CAP;
> > -		/* PSR2 needs frame sync as well */
> > -		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > +		dev_priv->psr.psr2_support = true;
> >  		DRM_DEBUG_KMS("PSR2 %s on sink",
> >  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> >  
> > @@ -269,11 +262,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  
> > -	/* Enable AUX frame sync at sink */
> > -	if (dev_priv->psr.aux_frame_sync)
> > -		drm_dp_dpcd_writeb(&intel_dp->aux,
> > -				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -				DP_AUX_FRAME_SYNC_ENABLE);
> >  	/* Enable ALPM at sink for psr2 */
> >  	if (dev_priv->psr.psr2_support && dev_priv->psr.alpm)
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > @@ -712,11 +700,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> >  		i915_reg_t psr_status;
> >  		u32 psr_status_mask;
> >  
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -					0);
> > -
> >  		if (dev_priv->psr.psr2_support) {
> >  			psr_status = EDP_PSR2_STATUS;
> >  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > @@ -860,10 +843,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
> >  		return;
> >  
> >  	if (HAS_DDI(dev_priv)) {
> > -		if (dev_priv->psr.aux_frame_sync)
> > -			drm_dp_dpcd_writeb(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
> > -					0);
> >  		if (dev_priv->psr.psr2_support) {
> >  			val = I915_READ(EDP_PSR2_CTL);
> >  			WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx