On Wed, 2018-12-12 at 05:02 -0800, Souza, Jose wrote: > On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan 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. > > > > > > ~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. > > The problem with this approach is if user wants to run just one test > the basic test will not run and DRRS will be kept on, it will not > fail > the no_drrs test but DRRS could interfere with other PSR tests. Disable DRRS for the other sub-tests, doesn't this work? echo 0 > /sys/kernel/debug/dri/0/i915_drrs_ctl > > If the call to disable display is moved to igt_fixture() it would > work > fine but in terms of modesets this approach have just one more > modeset > than forcing a modeset every time we write to PSR i915_edp_psr_debug, > for just the cost of one modeset in my opnion is better drop the > aditional PSR code path and just test the main one. > > > > > -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
