On Thu, 2018-11-15 at 12:57 -0800, Souza, Jose wrote: > On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote: > > Op 09-11-18 om 21:20 schreef José Roberto de Souza: > > > If panel supports DRRS and PSR and if driver is loaded without > > > PSR > > > enabled, driver will enable DRRS as expected but if PSR is > > > enabled > > > by > > > debugfs latter it will keep PSR and DRRS enabled causing possible > > > problems as DRRS will lower the refresh rate while PSR enabled. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 853e3f1370a0..bfc6a08b5cf4 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct > > > drm_i915_private *dev_priv, > > > > > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > > > > > - if (dev_priv->psr.prepared && enable) > > > + if (dev_priv->psr.prepared && enable) { > > > + if (crtc_state) > > > + intel_edp_drrs_disable(dp, crtc_state); > > > intel_psr_enable_locked(dev_priv, crtc_state); > > > + } > > > > > > mutex_unlock(&dev_priv->psr.lock); > > > return ret; > > > > I've considered this, but I thought it was a feature, not a bug. > > It's > > a pain to track > > how we handle this as intended. > > > > kms_frontbuffer_tracking is also controlling DRRS during the test, > > so > > perhaps simply > > fix the test? > > > > It seems the no_drrs test simply checks that if PSR is enabled, we > > don't have drrs > > enabled. We probably care about the default configuration, so I > > would > > simply disable > > the pipe, update the PSR flag, and then start running the tests. > > Else > > the only thing > > we test is that debugfs disables DRRS. Not that the default modeset > > path prevents > > PSR and DRRS simultaneously. > > > Yeah, I think we should force a modeset from debugs to test the > default > modeset path, fix the tests I think is a bad idea as it could leave > some test cases unhandled. Maarten, Can you comment on the Jose's suggestion above? I recall the idea behind your patches was to avoid modesets while enabling and disabling PSR. Jose's latest version otoh forces a modeset. -DK > > Also looking at DRRS it looks complete broken for page flips, it > would > kept set to DRRS_LOW forever. > > > > > > ~Maarten > > > > Maybe something like below? > > > > Perhaps move the drrs manipulation functions from > > kms_frontbuffer_tracking to lib/kms_psr.c > > > > ----8<------- > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c > > index 9767f475bf23..ffc356df06ce 100644 > > --- a/tests/kms_psr.c > > +++ b/tests/kms_psr.c > > @@ -414,9 +414,6 @@ int main(int argc, char *argv[]) > > kmstest_set_vt_graphics_mode(); > > data.devid = intel_get_drm_devid(data.drm_fd); > > > > - if (!data.with_psr_disabled) > > - psr_enable(data.debugfs_fd); > > - > > igt_require_f(sink_support(&data), > > "Sink does not support PSR\n"); > > > > @@ -428,18 +425,25 @@ int main(int argc, char *argv[]) > > } > > > > igt_subtest("basic") { > > - setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY); > > - igt_assert(psr_wait_entry_if_enabled(&data)); > > - test_cleanup(&data); > > - } > > + /* Disable display to get a default setup. */ > > + igt_display_commit2(&data.display, > > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > > + > > + if (!data.with_psr_disabled) > > + psr_enable(data.debugfs_fd); > > > > - igt_subtest("no_drrs") { > > setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY); > > igt_assert(psr_wait_entry_if_enabled(&data)); > > igt_assert(drrs_disabled(&data)); > > test_cleanup(&data); > > } > > > > + igt_fixture { > > + drrs_disable(); > > + > > + if (!data.with_psr_disabled) > > + psr_enable(data.debugfs_fd); > > + } > > + > > for (op = PAGE_FLIP; op <= RENDER; op++) { > > igt_subtest_f("primary_%s", op_str(op)) { > > data.op = op; > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
