2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@xxxxxxxx>: > 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@xxxxxxxxx> Tested-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Cc: Durgadoss R <durgadoss.r@xxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
