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. > > ~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); This makes a lot more sense to me, ensuring that DRRS does not get enabled in the default code path was the goal of the no-drrs test. -DK > } > > + 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
