On Thu, 2018-03-22 at 10:41 +0100, Maarten Lankhorst wrote: > Op 22-03-18 om 02:45 schreef Pandiyan, Dhinakaran: > > > > On Thu, 2018-03-15 at 11:28 +0100, Maarten Lankhorst wrote: > > > > > > Currently tests modify i915.enable_psr and then do a modeset > > > cycle > > > to change PSR. We can write a value to i915_edp_psr_status to > > > force > > > a certain value without a modeset. > > > > > > To retain compatibility with older userspace, we also still allow > > > the override through the module parameter, and add some tracking > > > to check whether a debugfs mode is specified. > > > > > While this is something we want to be able to do, I am concerned > > about > > adding more complexity to a feature that has barely been tested. > > > > How about doing a modeset before frontbuffer_tracking PSR subtests > > and > > one at the end? I'm assuming all of them are grouped together. > Currently we run all subtests individually, this means that we also > need to do > some extra modesets per test. One to disable PSR and collect the > reference CRC, the > other to enable PSR for the actual test. > > With these changes, we don't need to do so. > > > > > > Comments on this patch itself. > > 1) please split intel_psr_default_link_standby() into a separate > > patch. > Will do. Maarten, do you plan to rebase this patch? I would like to use this in the IGTs to enable PSR1 on PSR2 panels. -DK > > > > 2) how does the user know what values to write without looking at > > the > > code? > We match the modparam options for i915.enable_psr, but in general > user shouldn't touch it unless asked to. :) This is mostly meant for > IGT tests, > could also be useful for debugging i915 in general though. > > But if you really want, perhaps if we someone writes an invalid > value, we could also > output the possible values to debugfs? Though I don't think we ought > to. debugfs > doesn't always have documentation. > > > > 3) Can the connector and crtc be stored somewhere to avoid loops? > intel_psr_set_debugfs mode checks for idleness and waits for all > atomic commits to complete. > We need the HW state to toggle PSR, and this is the only way to > guarantee that crtc->state matches > the actual hw state. > > If we don't drop the psr lock, we would get a deadlock when > intel_psr_enable/disable is called from .crtc_disable, > because we wait for hw_done with psr lock already taken. > > > > 4) Has this been tested on any platforms with PSR? > I've had someone test v1 on a PSR capable system. It hung in the same > way as enabling PSR during boot did, > so that part is the same. > > For the later patches I used f2-cnl-alpha, but that system hangs a > few seconds / half a minute after loading i915. > Still gave me enough time to check we can write any value to debugfs. > > > > > 5) Do subtests need a finer control of PSR i.e., psr_exit() and > > psr_activate() instead of enable and disable > Not afaict. We sometimes invalidate the FB with dirtyfb, but that's > all the control we need I think. > > ~Maarten > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
