Re: [PATCH v2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug

From: "Souza, Jose" <jose.souza@intel.com>
To: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"maarten.lankhorst@linux.intel.com"
	<maarten.lankhorst@linux.intel.com>
Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH v2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
Date: Fri, 1 Feb 2019 02:03:17 +0000	[thread overview]
Message-ID: <42d583cbe0abe4aab8136a289d7e9a41b706510d.camel@intel.com> (raw)
In-Reply-To: <992008a3-0cc1-9755-ebb2-dded629c28de@linux.intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 2015 bytes --]

On Thu, 2019-01-31 at 10:34 +0100, Maarten Lankhorst wrote:
> Op 31-01-2019 om 01:58 schreef José Roberto de Souza:
> > Changing the i915_edp_psr_debug was enabling, disabling or
> > switching
> > PSR version by directly calling intel_psr_disable_locked() and
> > intel_psr_enable_locked(), what is not the default PSR path that
> > will
> > be executed by real users.
> > 
> > So lets force a fastset in the PSR CRTC to trigger a pipe update
> > and
> > stress the default code path.
> > 
> > Recently a bug was found when switching from PSR2 to PSR1 while
> > enable_psr kernel parameter was set to the default parameter, this
> > changes fix it and also fixes the bug linked bellow were DRRS was
> > left enabled together with PSR when enabling PSR from debugfs.
> > 
> > v2: Handling missing case: disabled to PSR1
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > 
> > Should I add IGT tests to test every state switch combination?
> 
> Should probably be done for DRRS as well. We should be able to stop
> having to set has_drrs and has_psr unconditionally then. :)
> Could be a separate followup patch.

I have DRRS/PSR bugs assigned to me, I will definitely fix that in the
future.

> 
> The complete duplication of the whole atomic state is overkill and
> should be avoided, just use 
> if (!intel_crtc_has_type(to_intel_crtc_state(crtc->state),
> INTEL_OUTPUT_EDP)
> 	continue;
> 
> crtc_state = drm_atomic_get_old_crtc_state(...)
> ....
> 
> And then do a normal commit. We will add all planes and connectors as
> needed.

Thanks for the suggestion, just sent the new version with this.

> 
> With that fixed:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx