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

[Souza, Jose]

On Wed, 2018-12-12 at 17:11 -0800, Dhinakaran Pandiyan wrote:
> 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

It would work but with this we are not testing or stressing any code
path that users will go through.

> > 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/20181213/735225d8/attachment-0001.sig>