[Intel-gfx] [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug

[Maarten Lankhorst]

Op 17-12-2018 om 12:16 schreef Maarten Lankhorst:
> Op 11-12-2018 om 19:16 schreef Souza, Jose:
>> On Tue, 2018-12-04 at 13:23 -0800, Dhinakaran Pandiyan wrote:
>>> On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
>>>> On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
>>>>> On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
>>>>>> On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
>>>>>>> On Thu, 2018-11-29 at 18:31 -0800, José Roberto de Souza
>>>>>>> wrote:
>>>>>>>> Changing the i915_edp_psr_debug was enabling, disabling or
>>>>>>>> switching
>>>>>>>> PSR version by directly calling intel_psr_disable_locked()
>>>>>>>> and
>>>>>>>> intel_psr_enable_locked(), what is not the default PSR path
>>>>>>>> that
>>>>>>>> is
>>>>>>>> executed in a regular modesets.
>>>>>>>>
>>>>>>>> So lets force a modeset in the PSR CRTC to trigger the
>>>>>>>> requested
>>>>>>>> PSR
>>>>>>>> state change and really stress the code path that matters
>>>>>>>> for
>>>>>>>> the
>>>>>>>> regular user.
>>>>>>>>
>>>>>>>> Also by doing this way it fixes the issue below, were DRRS
>>>>>>>> was
>>>>>>>> left
>>>>>>>> enabled together with PSR when enabling PSR from debugfs.
>>>>>>> While this patch does fix the issue, psr_compute_config() not
>>>>>>> checking
>>>>>>> crtc_state->has_drrs seems odd. We should change it to not
>>>>>>> set
>>>>>>> crtc_state->has_psr if crtc_state->has_drrs happens to be
>>>>>>> set.
>>>>>>> Or
>>>>>>> do
>>>>>>> it
>>>>>>> the other way around.
>>>>>> psr_compute_config() is not called when enabling PSR from
>>>>>> debugfs,
>>>>>> this
>>>>> Right. My suggestion is to allow either ->has_drrs or ->has_psr
>>>>> being
>>>>> set (not both) in the kernel and disable DRRS in the IGT before
>>>>> starting the test.
>>>> So in case were PSR is disabled by parameter and DRRS is supported
>>>> we
>>>> would not enable DRRS? Because has_psr is set even if PSR is
>>>> disabled.
>>> Set ->has_psr = true in psr_compute_config() only if the module
>>> parameter and debugfs mode allow it. That is how the code worked
>>> earlier. Given that this patch duplicates the atomic state and runs
>>> through all state checks, we can move back to the earlier way of
>>> completing all checks in psr_compute_config().
>>>
>>>> Disabling DRRS from IGT is duplicating the code that already do
>>>> that
>>>> and also not validating the default code path.
>>> Call drrs_compute_config() after psr_compute_config(), don't set
>>> has_drrs if has_psr is set.
>> What about add a flag to skip modeset so when running IGT tests we set
>> that flag and PSR mode will be changed in the next modeset, what is
>> already done after every write to i915_edp_psr_debug in IGT tests? This
>> way we remove the code duplication and only stress the default code
>> path.
>>
>> Also plus the changes in has_drrs that you mentioned but in other
>> patch.
> Well the reason we set both is because kernel should decide which one to enable. The
> only way we can have both active is if we mess with debugfs.
>
> I see the fact you're trying to enable both as a failure from the user. Anything in
> debugfs can be used for advanced debugging, and can possibly brick your system. If
> we really care, then we should disable DRRS when enabling PSR, and the same when
> enabling DRRS, that we disable PSR.
>
> There's no need to restore it afterwards, because it's debugfs api.
>
> ~Maarten
>
.. or we could stop messing around, make a commit which sets crtc_state->mode_changed,
it will degrade the modeset to a fastset, and then we can use update_pipe() to make
the new settings active:

https://patchwork.freedesktop.org/series/54339/

And then only set enable_psr enable_drrs in crtc_state when we plan to use

it. This removes the need for separate settings..

~Maarten