[7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe
diff mbox

Message ID 1434616228-28358-7-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter June 18, 2015, 8:30 a.m. UTC
The current code tracks business across all pipes, but we're only
really interested in the one pipe DRRS is enabled on. Fairly tiny
optimization, but something I noticed while reading the code. But it
might matter a bit when e.g. showing a video or something only on the
external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take
appropriate actions.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Paulo Zanoni June 23, 2015, 7:57 p.m. UTC | #1
2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code tracks business across all pipes, but we're only
> really interested in the one pipe DRRS is enabled on. Fairly tiny

s/DRRS/PSR/

> optimization, but something I noticed while reading the code. But it
> might matter a bit when e.g. showing a video or something only on the
> external screen, while the panel is kept static.
>
> Also regroup the code slightly: First compute new bitmasks, then take
> appropriate actions.

One of the ideas I had last year was to implement a way to tell user
space (possibly through debugs) when FBC/PSR gets enabled/disabled. My
initial idea was to do this through timestamps. Our IGT suite would
then verify those timestamps: if we have 2 pipes enabled, then we
write on the non-FBC buffer, the FBC timestamps can't change. This
type of test would catch exactly the bug you're solving here.

I even implemented this, and added support for it on
kms_frontbuffer_tracking, but I ended never sending the Kernel patch
to the list due to the possible slowness created by the constant
timestamp generation. Maybe I should resurrect this and hide it under
some debug kconfig option. I also considered maybe moving the
implementation from timestamps debugfs file events, or maybe tracing
code, or something else. Ideas and bikesheds are welcome here.

So I added some debug messages to the PSR code, then I ran:
sudo ./kms_frontbuffer_tracking --run-subtest
psr-2p-scndscrn-pri-sfb-draw-mmap-cpu --step --step

And I kept watching dmesg as I stepped. Without the patch, I could see
psr being disabled/enabled as we were writing on the secondary screen
frontbuffer. With the patch, we stop calling intel_psr_exit()! So:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5ee0fa57ed19..e354ceacb628 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -663,11 +663,12 @@ void intel_psr_invalidate(struct drm_device *dev,
>         crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
>
> -       intel_psr_exit(dev);
> -
>         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> -
>         dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
> +
> +       if (frontbuffer_bits)
> +               intel_psr_exit(dev);
> +
>         mutex_unlock(&dev_priv->psr.lock);
>  }
>
> @@ -698,6 +699,8 @@ void intel_psr_flush(struct drm_device *dev,
>
>         crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
> +
> +       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
>         /*
> @@ -716,7 +719,7 @@ void intel_psr_flush(struct drm_device *dev,
>          * invalidating. Which means we need to manually fake this in
>          * software for all flushes, not just when we've seen a preceding
>          * invalidation through frontbuffer rendering. */
> -       if (!HAS_DDI(dev))
> +       if (frontbuffer_bits && !HAS_DDI(dev))
>                 intel_psr_exit(dev);
>
>         if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter June 23, 2015, 9 p.m. UTC | #2
On Tue, Jun 23, 2015 at 04:57:26PM -0300, Paulo Zanoni wrote:
> 2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The current code tracks business across all pipes, but we're only
> > really interested in the one pipe DRRS is enabled on. Fairly tiny
> 
> s/DRRS/PSR/
> 
> > optimization, but something I noticed while reading the code. But it
> > might matter a bit when e.g. showing a video or something only on the
> > external screen, while the panel is kept static.
> >
> > Also regroup the code slightly: First compute new bitmasks, then take
> > appropriate actions.
> 
> One of the ideas I had last year was to implement a way to tell user
> space (possibly through debugs) when FBC/PSR gets enabled/disabled. My
> initial idea was to do this through timestamps. Our IGT suite would
> then verify those timestamps: if we have 2 pipes enabled, then we
> write on the non-FBC buffer, the FBC timestamps can't change. This
> type of test would catch exactly the bug you're solving here.
> 
> I even implemented this, and added support for it on
> kms_frontbuffer_tracking, but I ended never sending the Kernel patch
> to the list due to the possible slowness created by the constant
> timestamp generation. Maybe I should resurrect this and hide it under
> some debug kconfig option. I also considered maybe moving the
> implementation from timestamps debugfs file events, or maybe tracing
> code, or something else. Ideas and bikesheds are welcome here.
> 
> So I added some debug messages to the PSR code, then I ran:
> sudo ./kms_frontbuffer_tracking --run-subtest
> psr-2p-scndscrn-pri-sfb-draw-mmap-cpu --step --step
> 
> And I kept watching dmesg as I stepped. Without the patch, I could see
> psr being disabled/enabled as we were writing on the secondary screen
> frontbuffer. With the patch, we stop calling intel_psr_exit()! So:

I think there's two cases really: Adding tracepoints to watch the psr or
frontbuffer code could certainly be useful.

For testcases otoh I'm not in favour of coupling them tightly with the
current kernel implementation. If we only check behavior (like residency
or captured CRCs) we can change the kernel implementation (like e.g.
switch away from hw tracking or the other way round) without any changes
to testcases. If we couple tests tightly with such tracepoints/timestamps
then we might accidentally break the testcase while changing it.

And since upstream is really big about never breaking ABI (which means
observable behaviour, not never changing how it's implemented) I think
doing blackbox testcase is a big plus.

But if it's really not possibel to validate a feature without
introspection then I'm ok with adding tracepoints/ts or similar things.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5ee0fa57ed19..e354ceacb628 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -663,11 +663,12 @@  void intel_psr_invalidate(struct drm_device *dev,
 	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
-	intel_psr_exit(dev);
-
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
-
 	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
+
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
+
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -698,6 +699,8 @@  void intel_psr_flush(struct drm_device *dev,
 
 	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
 	/*
@@ -716,7 +719,7 @@  void intel_psr_flush(struct drm_device *dev,
 	 * invalidating. Which means we need to manually fake this in
 	 * software for all flushes, not just when we've seen a preceding
 	 * invalidation through frontbuffer rendering. */
-	if (!HAS_DDI(dev))
+	if (frontbuffer_bits && !HAS_DDI(dev))
 		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)