[Intel-gfx] [PATCH 7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe
Paulo Zanoni
przanoni at gmail.com
Tue Jun 23 12:57:26 PDT 2015
2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter at 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 at intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Durgadoss R <durgadoss.r at intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list