[Intel-gfx] [PATCH] drm/i915: Don't do pre plane update on disabled crtcs

[Maarten Lankhorst]

Op 19-01-16 om 20:22 schreef Ville Syrjälä:
> On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote:
>> Op 14-01-16 om 17:52 schreef Ville Syrjälä:
>>> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote:
>>>> CI/Bat got following (shortened) trace on byt and also
>>>> on bsw:
>>>>
>>>> ------------[ cut here ]-----------
>>>> Unclaimed register detected before reading register 0x186500
>>>> Call Trace:
>>>>  __unclaimed_reg_debug+0x68/0x80 [i915]
>>>> vlv_read32+0x2de/0x370 [i915]
>>>> intel_set_memory_cxsr+0x87/0x1a0 [i915]
>>>> intel_pre_plane_update+0xb3/0xf0 [i915]
>>>> intel_atomic_commit+0x3b5/0x17c0 [i915]
>>>> ...
>>>> ---[ end trace 6387a0ad001bb39f ]---
>>>>
>>>> Fix this by limiting pre plane update only to active crtcs.
>>>>
>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index aa24f79d85bf..a134a698d97d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>  		if (!needs_modeset(crtc->state))
>>>>  			continue;
>>>>  
>>>> -		intel_pre_plane_update(intel_crtc);
>>>> -
>>>>  		if (crtc_state->active) {
>>>> +			intel_pre_plane_update(intel_crtc);
>>> I think you'll want to deal with the other one too (the one in the crtc
>>> enable/plane update path). Actually I think the plane update stuff
>>> should be split into a separate loop from the crtc_enable stuff, but
>>> that's a separate topic.
>>>
>>> Hmm. And there's a post_plane_update there that looks a bit too
>>> lonely as well. Something really should be done to make this code
>>> less convoluted. A modeset/plane update shouldn't be this hard to
>>> get right.
>>>
>>>
>> I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does?
> Staring at the code a bit. I would assume it's due to
> intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true'
> when the plane gets either turned off or on.
>
> This sort of stuff makes me wish again that we had a separate atomic state
> for the disable case. Using the same state flag for both the disable
> and enable phases of the operation is very confusing. Would be even
> more confusing if we required that flag to have different values for the
> disable and enable phases.
But cxsr_allowed still needs to be false. Maybe this?

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5abe97c1a944..bfdf02f7436b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4830,7 +4830,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 
 	if (pipe_config->disable_cxsr) {
 		crtc->wm.cxsr_allowed = false;
-		intel_set_memory_cxsr(dev_priv, false);
+		if (!needs_modeset(&pipe_config->base))
+			intel_set_memory_cxsr(dev_priv, false);
 	}
 
 	/*