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

From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe
Date: Tue, 23 Jun 2015 23:00:59 +0200	[thread overview]
Message-ID: <20150623210059.GL25769@phenom.ffwll.local> (raw)
In-Reply-To: <CA+gsUGRP4oBPfrDhFvNHas6UVugBoUBcG_PHSwBrgxGtCz-Ufw@mail.gmail.com>

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