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