| Message ID | 1434616228-28358-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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)
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(-)