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

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx