Re: [PATCH] drm/i915: Allow control of PSR at runtime through debugfs.

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx