[Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Dhinakaran Pandiyan]

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 at linux.intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >  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;
> > >