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@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: 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
