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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.
> 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