[PATCH 1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[PATCH 1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Dhinakaran Pandiyan]

The hardware can start selective update following capture of a full frame
in the remote frame buffer, there is no need to wait any longer. Set
"Frames Before SU Entry" bitfield to the default value of 1.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 83528647b40b..105b7ea2cd98 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -424,6 +424,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
 	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
+	val |= EDP_PSR2_FRAME_BEFORE_SU(1);
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
@@ -432,8 +433,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		val |= EDP_Y_COORDINATE_ENABLE;
 
-	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
-
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
 	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
 		val |= EDP_PSR2_TP2_TIME_50us;
-- 
2.14.1

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

[PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Dhinakaran Pandiyan]

PSR2 sinks that require Y coordinates for selective update also need the
Y coordinate Valid bit in VSC SDP.
Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-12)

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 105b7ea2cd98..92672954dfef 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * good enough. */
 	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-		val |= EDP_Y_COORDINATE_ENABLE;
+		val |= EDP_Y_COORDINATE_ENABLE | EDP_Y_COORDINATE_VALID;
 
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
 	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
-- 
2.14.1

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

✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"
URL   : https://patchwork.freedesktop.org/series/50593/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/psr: Reduce PSR2 "frames before selective update entry"
-O:drivers/gpu/drm/i915/intel_psr.c:425:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:425:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:425:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:425:23: warning: expression using sizeof(void)

Commit: drm/i915/psr: Set Y coordinate valid for Gen10+ display
Okay!

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"
URL   : https://patchwork.freedesktop.org/series/50593/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4933 -> Patchwork_10371 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50593/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10371 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (44 -> 40) ==

  Additional (2): fi-glk-j4005 fi-snb-2520m 
  Missing    (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-icl-u2 fi-ctg-p8600 fi-gdg-551 


== Build changes ==

    * Linux: CI_DRM_4933 -> Patchwork_10371

  CI_DRM_4933: 6b7a44d1597791524f46d7ea17620db54dffdc8c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10371: 32fc1b6c3261b803afd70572b42479068f73d1f0 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

32fc1b6c3261 drm/i915/psr: Set Y coordinate valid for Gen10+ display
41e2255b028b drm/i915/psr: Reduce PSR2 "frames before selective update entry"

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10371/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"
URL   : https://patchwork.freedesktop.org/series/50593/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4933_full -> Patchwork_10371_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10371_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10371_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10371_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_draw_crc@draw-method-xrgb8888-render-untiled:
      shard-skl:          PASS -> FAIL

    
    ==== Warnings ====

    igt@drm_read@empty-nonblock:
      shard-snb:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in Patchwork_10371_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-apl:          PASS -> INCOMPLETE (fdo#106886, fdo#103927)

    igt@gem_exec_flush@basic-batch-kernel-default-wb:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-skl:          NOTRUN -> TIMEOUT (fdo#108039)

    igt@gem_softpin@noreloc-s3:
      shard-skl:          PASS -> INCOMPLETE (fdo#107773, fdo#104108)

    igt@kms_busy@extended-pageflip-hang-newfb-render-a:
      shard-apl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-128x128-sliding:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-256x256-onscreen:
      shard-glk:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108)

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          PASS -> FAIL (fdo#105767)

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763, fdo#106538)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-mmap-gtt:
      shard-skl:          PASS -> FAIL (fdo#103167)

    {igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb}:
      shard-apl:          PASS -> FAIL (fdo#108145)

    {igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb}:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +2

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166)
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_rotation_crc@exhaust-fences:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#105748)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@kms_sysfs_edid_timing:
      shard-skl:          NOTRUN -> FAIL (fdo#100047)

    
    ==== Possible fixes ====

    igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-b:
      shard-apl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +1

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          FAIL (fdo#103232) -> PASS +1
      shard-apl:          FAIL (fdo#103191, fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-64x64-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_plane@plane-position-covered-pipe-b-planes:
      shard-apl:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS +3

    igt@pm_rpm@pm-tiling:
      shard-skl:          INCOMPLETE (fdo#107807) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105748 https://bugs.freedesktop.org/show_bug.cgi?id=105748
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4933 -> Patchwork_10371

  CI_DRM_4933: 6b7a44d1597791524f46d7ea17620db54dffdc8c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10371: 32fc1b6c3261b803afd70572b42479068f73d1f0 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10371/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Rodrigo Vivi]

On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote:
> PSR2 sinks that require Y coordinates for selective update also need the
> Y coordinate Valid bit in VSC SDP.
> Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-12)

I couldn't get any meaningful information about Y coordinate valid bit
looking at this table...

what am I missing?

> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 105b7ea2cd98..92672954dfef 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 * good enough. */
>  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> -		val |= EDP_Y_COORDINATE_ENABLE;
> +		val |= EDP_Y_COORDINATE_ENABLE | EDP_Y_COORDINATE_VALID;

But also, this seems to be doing the opposite what you wrote on the
commit message since this bit means:

"Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)

>  
>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
>  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> -- 
> 2.14.1
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote:
> On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote:
> > PSR2 sinks that require Y coordinates for selective update also
> > need the
> > Y coordinate Valid bit in VSC SDP.
> > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-
> > 12)
> 
> I couldn't get any meaningful information about Y coordinate valid
> bit
> looking at this table...
> 
> what am I missing?
"
Y-Coordinate_Valid
If the Sink device indicates that Y-coordinate is required, the Source
device must program HB2 (VSC Revision Number) to 04h or 05h.
Additionally, the Source device must set this bit to 1 to indicate that
the Y-coordinate provided in DB12 through DB13 is valid. However, if
the Y-coordinate provided in DB12 through DB13 is not valid, this bit
must be cleared to 0 (this includes when DB12 through DB13 are not set
and remain at a default value of 0000h, because a Y-coordinate value of
0000h represents the first active video line)."

We enable PSR2 only when the sink says Y-coordinate is required and the
hardware needs to be told to send Y coordinate valid bit in the VSC
packets for such sinks.


> 
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 105b7ea2cd98..92672954dfef 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	 * good enough. */
> >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > -		val |= EDP_Y_COORDINATE_ENABLE;
> > +		val |= EDP_Y_COORDINATE_ENABLE |
> > EDP_Y_COORDINATE_VALID;
> 
> But also, this seems to be doing the opposite what you wrote on the
> commit message since this bit means:
> 
> "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)

Oops! You are right, the bit name threw me off.


> >  
> >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > -- 
> > 2.14.1
> > 
> > _______________________________________________
> > 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

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Souza, Jose]

On Fri, 2018-10-05 at 10:51 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote:
> > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan
> > wrote:
> > > PSR2 sinks that require Y coordinates for selective update also
> > > need the
> > > Y coordinate Valid bit in VSC SDP.
> > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-
> > > 12)
> > 
> > I couldn't get any meaningful information about Y coordinate valid
> > bit
> > looking at this table...
> > 
> > what am I missing?
> "
> Y-Coordinate_Valid
> If the Sink device indicates that Y-coordinate is required, the
> Source
> device must program HB2 (VSC Revision Number) to 04h or 05h.
> Additionally, the Source device must set this bit to 1 to indicate
> that
> the Y-coordinate provided in DB12 through DB13 is valid. However, if
> the Y-coordinate provided in DB12 through DB13 is not valid, this bit
> must be cleared to 0 (this includes when DB12 through DB13 are not
> set
> and remain at a default value of 0000h, because a Y-coordinate value
> of
> 0000h represents the first active video line)."
> 
> We enable PSR2 only when the sink says Y-coordinate is required and
> the
> hardware needs to be told to send Y coordinate valid bit in the VSC
> packets for such sinks.
> 
> 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 105b7ea2cd98..92672954dfef 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	 * good enough. */
> > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > -		val |= EDP_Y_COORDINATE_ENABLE;
> > > +		val |= EDP_Y_COORDINATE_ENABLE |
> > > EDP_Y_COORDINATE_VALID;
> > 
> > But also, this seems to be doing the opposite what you wrote on the
> > commit message since this bit means:
> > 
> > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)
> 
> Oops! You are right, the bit name threw me off.

When I added this bit I first named it "EDP_Y_COORDINATE_INVALID" but
was decided to go back to BSpec name.

> 
> 
> > >  
> > >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> > >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > 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

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Rodrigo Vivi]

On Fri, Oct 05, 2018 at 10:51:28AM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote:
> > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan wrote:
> > > PSR2 sinks that require Y coordinates for selective update also
> > > need the
> > > Y coordinate Valid bit in VSC SDP.
> > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table 6-
> > > 12)
> > 
> > I couldn't get any meaningful information about Y coordinate valid
> > bit
> > looking at this table...
> > 
> > what am I missing?
> "
> Y-Coordinate_Valid
> If the Sink device indicates that Y-coordinate is required, the Source
> device must program HB2 (VSC Revision Number) to 04h or 05h.
> Additionally, the Source device must set this bit to 1 to indicate that
> the Y-coordinate provided in DB12 through DB13 is valid. However, if
> the Y-coordinate provided in DB12 through DB13 is not valid, this bit
> must be cleared to 0 (this includes when DB12 through DB13 are not set
> and remain at a default value of 0000h, because a Y-coordinate value of
> 0000h represents the first active video line)."

Interesting... I keep forgetting I need to delete my old spec and download
a new one from VESA website...

On the one that I have here I couldn't find this text and also HB2 doesn't
have these options:

4:0 Revision Number
01h = VSC packet supports only 3D stereo.
02h = 3D stereo + PSR1.
03h = 3D stereo + PSR2.
7:5 RESERVED

> 
> We enable PSR2 only when the sink says Y-coordinate is required and the
> hardware needs to be told to send Y coordinate valid bit in the VSC
> packets for such sinks.
> 
> 
> > 
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 105b7ea2cd98..92672954dfef 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	 * good enough. */
> > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > -		val |= EDP_Y_COORDINATE_ENABLE;
> > > +		val |= EDP_Y_COORDINATE_ENABLE |
> > > EDP_Y_COORDINATE_VALID;
> > 
> > But also, this seems to be doing the opposite what you wrote on the
> > commit message since this bit means:
> > 
> > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)
> 
> Oops! You are right, the bit name threw me off.

yeap... the name is so bad indeed.

> 
> 
> > >  
> > >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> > >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > 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

Re: [PATCH 1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Souza, Jose]

On Thu, 2018-10-04 at 20:01 -0700, Dhinakaran Pandiyan wrote:
> The hardware can start selective update following capture of a full
> frame
> in the remote frame buffer, there is no need to wait any longer. Set
> "Frames Before SU Entry" bitfield to the default value of 1.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 83528647b40b..105b7ea2cd98 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -424,6 +424,7 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  
>  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> + 1);
>  	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> +	val |= EDP_PSR2_FRAME_BEFORE_SU(1);

I guess 0 would the right value, setting to 1 feels like it would wait
1 frame after a flip/front buffer modfication to do a SU. I will run
some tests changing EDP_PSR2_IDLE_FRAME_SHIFT and
EDP_PSR2_FRAME_BEFORE_SU.

>  
>  	/* FIXME: selective update is probably totally broken because
> it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone
> isn't
> @@ -432,8 +433,6 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency 
> + 1);
> -
>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
>  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
>  		val |= EDP_PSR2_TP2_TIME_50us;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 12:54 -0700, Rodrigo Vivi wrote:
> On Fri, Oct 05, 2018 at 10:51:28AM -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote:
> > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > PSR2 sinks that require Y coordinates for selective update also
> > > > need the
> > > > Y coordinate Valid bit in VSC SDP.
> > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table
> > > > 6-
> > > > 12)
> > > 
> > > I couldn't get any meaningful information about Y coordinate
> > > valid
> > > bit
> > > looking at this table...
> > > 
> > > what am I missing?
> > 
> > "
> > Y-Coordinate_Valid
> > If the Sink device indicates that Y-coordinate is required, the
> > Source
> > device must program HB2 (VSC Revision Number) to 04h or 05h.
> > Additionally, the Source device must set this bit to 1 to indicate
> > that
> > the Y-coordinate provided in DB12 through DB13 is valid. However,
> > if
> > the Y-coordinate provided in DB12 through DB13 is not valid, this
> > bit
> > must be cleared to 0 (this includes when DB12 through DB13 are not
> > set
> > and remain at a default value of 0000h, because a Y-coordinate
> > value of
> > 0000h represents the first active video line)."
> 
> Interesting... I keep forgetting I need to delete my old spec and
> download
> a new one from VESA website...
> 
> On the one that I have here I couldn't find this text and also HB2
> doesn't
> have these options:
> 
> 4:0 Revision Number
> 01h = VSC packet supports only 3D stereo.
> 02h = 3D stereo + PSR1.
> 03h = 3D stereo + PSR2.
> 7:5 RESERVED

This got added in eDP 1.4a.


> 
> > 
> > We enable PSR2 only when the sink says Y-coordinate is required and
> > the
> > hardware needs to be told to send Y coordinate valid bit in the VSC
> > packets for such sinks.
> > 
> > 
> > > 
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 105b7ea2cd98..92672954dfef 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	 * good enough. */
> > > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > > >  	if (INTEL_GEN(dev_priv) >= 10 ||
> > > > IS_GEMINILAKE(dev_priv))
> > > > -		val |= EDP_Y_COORDINATE_ENABLE;
> > > > +		val |= EDP_Y_COORDINATE_ENABLE |
> > > > EDP_Y_COORDINATE_VALID;
> > > 
> > > But also, this seems to be doing the opposite what you wrote on
> > > the
> > > commit message since this bit means:
> > > 
> > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)
> > 
> > Oops! You are right, the bit name threw me off.
> 
> yeap... the name is so bad indeed.
> 
> > 
> > 
> > > >  
> > > >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> > > >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > > > -- 
> > > > 2.14.1
> > > > 
> > > > _______________________________________________
> > > > 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

Re: [PATCH 1/2] drm/i915/psr: Reduce PSR2 "frames before selective update entry"

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 13:00 -0700, Souza, Jose wrote:
> On Thu, 2018-10-04 at 20:01 -0700, Dhinakaran Pandiyan wrote:
> > The hardware can start selective update following capture of a full
> > frame
> > in the remote frame buffer, there is no need to wait any longer.
> > Set
> > "Frames Before SU Entry" bitfield to the default value of 1.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 83528647b40b..105b7ea2cd98 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -424,6 +424,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  
> >  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > + 1);
> >  	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > +	val |= EDP_PSR2_FRAME_BEFORE_SU(1);
> 
> I guess 0 would the right value, setting to 1 feels like it would
> wait
> 1 frame after a flip/front buffer modfication to do a SU. I will run
> some tests changing EDP_PSR2_IDLE_FRAME_SHIFT and
> EDP_PSR2_FRAME_BEFORE_SU.
If that was the case, we should have seen noticeable lags with the
current value of 6? And I can't tell why there would be a configurable
delay to update a new frame.

I believe this is just like PSR1 idle frames, the field allows the
driver to configure the number of idle frames before entering the SU
mode.

-DK


> 
> >  
> >  	/* FIXME: selective update is probably totally broken because
> > it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > isn't
> > @@ -432,8 +433,6 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> >  
> > -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-
> > >psr.sink_sync_latency 
> > + 1);
> > -
> >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> >  		val |= EDP_PSR2_TP2_TIME_50us;

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

Re: [PATCH 2/2] drm/i915/psr: Set Y coordinate valid for Gen10+ display

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 12:53 -0700, Souza, Jose wrote:
> On Fri, 2018-10-05 at 10:51 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 10:38 -0700, Rodrigo Vivi wrote:
> > > On Thu, Oct 04, 2018 at 08:01:30PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > PSR2 sinks that require Y coordinates for selective update also
> > > > need the
> > > > Y coordinate Valid bit in VSC SDP.
> > > > Spec: eDP 1.4b VSC payload extension for PSR2 operation (Table
> > > > 6-
> > > > 12)
> > > 
> > > I couldn't get any meaningful information about Y coordinate
> > > valid
> > > bit
> > > looking at this table...
> > > 
> > > what am I missing?
> > 
> > "
> > Y-Coordinate_Valid
> > If the Sink device indicates that Y-coordinate is required, the
> > Source
> > device must program HB2 (VSC Revision Number) to 04h or 05h.
> > Additionally, the Source device must set this bit to 1 to indicate
> > that
> > the Y-coordinate provided in DB12 through DB13 is valid. However,
> > if
> > the Y-coordinate provided in DB12 through DB13 is not valid, this
> > bit
> > must be cleared to 0 (this includes when DB12 through DB13 are not
> > set
> > and remain at a default value of 0000h, because a Y-coordinate
> > value
> > of
> > 0000h represents the first active video line)."
> > 
> > We enable PSR2 only when the sink says Y-coordinate is required and
> > the
> > hardware needs to be told to send Y coordinate valid bit in the VSC
> > packets for such sinks.
> > 
> > 
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan@intel.com
> > > > > 
> > > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 105b7ea2cd98..92672954dfef 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -431,7 +431,7 @@ static void hsw_activate_psr2(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	 * good enough. */
> > > >  	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> > > >  	if (INTEL_GEN(dev_priv) >= 10 ||
> > > > IS_GEMINILAKE(dev_priv))
> > > > -		val |= EDP_Y_COORDINATE_ENABLE;
> > > > +		val |= EDP_Y_COORDINATE_ENABLE |
> > > > EDP_Y_COORDINATE_VALID;
> > > 
> > > But also, this seems to be doing the opposite what you wrote on
> > > the
> > > commit message since this bit means:
> > > 
> > > "Do not include Y-coordinate valid eDP 1.4" (Bspec: 7713)
> > 
> > Oops! You are right, the bit name threw me off.
> 
> When I added this bit I first named it "EDP_Y_COORDINATE_INVALID" but
> was decided to go back to BSpec name.

I remember it now, something like EDP_Y_COORDINATE_VALID_DISABLE would
be more appropriate. 

-DK


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