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

[Souza, Jose]

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.

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;
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20181212/557be47c/attachment-0001.sig>