Selective update testing
play a video and check 0x60094 , (03, 06) will indicated that system is in su state.
-----Original Message-----
From: Vivi, Rodrigo
Sent: Wednesday, March 28, 2018 3:07 AM
To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx>
Cc: Souza, Jose <jose.souza@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nagaraju, Vathsala <vathsala.nagaraju@xxxxxxxxx>
Subject: Re: [PATCH 04/12] drm/i915/psr: Tie PSR2 support to Y coordinate requirement
On Fri, Mar 23, 2018 at 07:34:34PM -0700, Pandiyan, Dhinakaran wrote:
>
> On Fri, 2018-03-23 at 23:51 +0000, Souza, Jose wrote:
> > On Fri, 2018-03-23 at 15:59 -0700, Pandiyan, Dhinakaran wrote:
> > >
> > >
> > > On Thu, 2018-03-22 at 14:48 -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@xxxxxxxxx>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.h | 1 -
> > > > drivers/gpu/drm/i915/intel_psr.c | 46
> > > > +++++++++++++++++-----------
> > > > ------------
> > > > 2 files changed, 19 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h index
> > > > 7fe00509e51a..cce32686fd75 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 d46320a451d9..23f38ab10636 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;
> > > >
> > > > @@ -130,22 +130,29 @@ void intel_psr_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > > drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > > > >psr_dpcd,
> > > > sizeof(intel_dp->psr_dpcd));
> > > >
> > > > - if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > > > + if (intel_dp->psr_dpcd[0]) {
> > > > dev_priv->psr.sink_support = true;
> > > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > > > }
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 9 &&
> > > > - (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > > -
> > > > - dev_priv->psr.sink_support = true;
> > > > - dev_priv->psr.psr2_support = true;
> > > > + (intel_dp->psr_dpcd[0] ==
> > > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > > > + /*
> > > > + * All panels that supports PSR version 03h (PSR2
> > > > +
> > > > + * Y-coordinate) 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
> > > > + * without a aux frame sync.
> > >
> > > Can you cite this from the spec please? just the "panel is capable
> > > to do selective update without a aux frame sync" part. Or you
> > > could just remove that line so that we can get this patch reviewed
> > > and merged.
> >
> > There is no such statement in spec like this, the closest that it
> > have
> > is:
> > "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."
> >
> > But we know that this works by all the previous tests when enabling
> > PSR2 and I also tested it.
>
> Please do update the comment to say so. Do you know if these tests for
> PSR2 selective update are in IGT? Or were these manual tests?
>
> Rodrigo/Vathsala, any idea how selective update was and can be tested?
nope :(
>
> The patch as such makes sense to me
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> if
> you update the comment.
>
>
> >
> > >
> > >
> > > > + *
> > > > + * To support PSR version 02h and PSR version 03h
> > > > without
> > > > + * Y-coordinate requirement panels we would need
> > > > to enable
> > > > + * GTC first.
> > > > + */
> > > > + dev_priv->psr.psr2_support =
> > > > intel_dp_get_y_coord_required(intel_dp);
> > > > DRM_DEBUG_KMS("PSR2 %s on sink",
> > > > dev_priv->psr.psr2_support ?
> > > > "supported" : "not supported");
> > > >
> > > > if (dev_priv->psr.psr2_support) {
> > > > - dev_priv->psr.y_cord_support =
> > > > - intel_dp_get_y_cord_status(intel_d
> > > > p);
> > > > dev_priv->psr.colorimetry_support =
> > > > intel_dp_get_colorimetry_status(in
> > > > tel_dp);
> > > > dev_priv->psr.alpm =
> > > > @@ -191,16 +198,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 */
> > > > @@ -458,15 +461,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;
> > > > }
> > > >
> > > > @@ -566,7 +560,6 @@ 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);
> > > >
> > > > @@ -577,9 +570,8 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > > hsw_psr_setup_aux(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,
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
