| Message ID | 20180322214848.28022-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > 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) > --- > 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
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 > > 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) Okay, I will wait Vathsala comments to send another version. Thanks > > > --- > > 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_SYN > > C_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_S > > YNC_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_S > > YNC_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
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
On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote: > 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? There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE register in sink, I'm reading this as valid but the value of aux frame in sink is always 0, the same as GTC_LIVE register in source side by this I guess source is sending 0 in each aux frame sync. > > > > 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 I agree with your interpretation of the spec but by the previous attempts of enabling of PSR/PSR2 and also by my tests the selective update with y-coordinate works at least in the pannels that we have access to. And do not makes sense enable any PSR2 if we are going to do full frame updates, that is PSR1. > > > > > > --- > > > 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_S > > > YNC_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_CO > > > NF, > > > - 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
On Fri, 2018-03-23 at 23:49 +0000, Souza, Jose wrote: > On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote: > > 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? > > There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE > register in sink, I'm reading this as valid but the value of aux frame > in sink is always 0, the same as GTC_LIVE register in source side by > this I guess source is sending 0 in each aux frame sync. > > > > > > > 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 > > I agree with your interpretation of the spec but by the previous > attempts of enabling of PSR/PSR2 and also by my tests the selective > update with y-coordinate works at least in the pannels that we have > access to. I'll take your word on that. How are you checking selective update is indeed working? Vathsala/Rodrigo Any idea why selective update seems to be working without aux frame sync? > And do not makes sense enable any PSR2 if we are going to do full frame > updates, that is PSR1. Yeah, but we do want to make sure selective update is working without aux frame sync. As the driver doesn't implement aux frame sync, there's not much meaning in requiring the sink to support it. So, this patch is Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Please rewrite the commit message stating what's in the spec and include the contradictory observation you made in the tests. Thanks for all the work on this patch. -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_S > > > > YNC_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); > > > > > > > > ^ Extra line > > > > - /* 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_CO > > > > NF, > > > > - 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
On Fri, 2018-03-23 at 19:16 -0700, Pandiyan, Dhinakaran wrote: > On Fri, 2018-03-23 at 23:49 +0000, Souza, Jose wrote: > > On Fri, 2018-03-23 at 15:14 -0700, Pandiyan, Dhinakaran wrote: > > > 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? > > > > There this a this bit AUX_FRAME_SYNC Valid in AUX_FRAME_SYNC VALUE > > register in sink, I'm reading this as valid but the value of aux > > frame > > in sink is always 0, the same as GTC_LIVE register in source side > > by > > this I guess source is sending 0 in each aux frame sync. > > > > > > > > > > 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 > > > > I agree with your interpretation of the spec but by the previous > > attempts of enabling of PSR/PSR2 and also by my tests the selective > > update with y-coordinate works at least in the pannels that we have > > access to. > > I'll take your word on that. How are you checking selective update is > indeed working? Doing a lot of reads to i915_edp_psr_status debugfs with all the patches in this series, I can see sink status as "active receiving selective update" and on the SDP I can see that the Y-Coordinate is valid and the image on sink is alright. > > Vathsala/Rodrigo > > Any idea why selective update seems to be working without aux frame > sync? > > > And do not makes sense enable any PSR2 if we are going to do full > > frame > > updates, that is PSR1. > > Yeah, but we do want to make sure selective update is working without > aux frame sync. > > As the driver doesn't implement aux frame sync, there's not much > meaning > in requiring the sink to support it. So, this patch is > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Please rewrite the commit message stating what's in the spec and > include > the contradictory observation you made in the tests. Thanks for all > the > work on this patch. Sure. Changing it to: eDP spec states that aux frame is required to do PSR2 selective update but i915 don't fully implement it. It sends the aux frame sync messages but the value is always zero as the GTC is not enabled in driver. Through tests was findout that pannels can do selective update when the y-coordinate is also included in SDP, that is why it is required to run PSR2 in i915. A dummy value is not useful at all to sink, so removing everything related to aux frame sync, if GTC is enabled we can bring this back. > > -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_FRA > > > > > ME_S > > > > > YNC_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); > > > > > > > > > > > > ^ Extra line Nice catch. > > > > > > - /* 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_SYN > > > > > C_CO > > > > > NF, > > > > > - 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_F > > > > > RAME > > > > > _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_F > > > > > RAME > > > > > _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
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));
Without GTC enabled hardware is sending dummy aux frame sync value 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> 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 | 23 +---------------------- 2 files changed, 1 insertion(+), 23 deletions(-)