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

[Dhinakaran Pandiyan]

On Thu, 2018-11-15 at 12:57 -0800, Souza, Jose 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.
> 
> 
> Yeah, I think we should force a modeset from debugs to test the
> default
> modeset path, fix the tests I think is a bad idea as it could leave
> some test cases unhandled.

Maarten,

Can you comment on the Jose's suggestion above? I recall the idea
behind your patches was to avoid modesets while enabling and disabling
PSR. Jose's latest version otoh forces a modeset.

-DK

> 
> Also looking at DRRS it looks complete broken for page flips, it
> would
> kept set to DRRS_LOW forever.
> 
> 
> > 
> > ~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);
> >  	}
> >  
> > +	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;
> >