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

[Souza, Jose]

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.

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;
> 
-------------- 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/20181115/fce170f5/attachment.sig>