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

[Mika Kuoppala]

Maarten Lankhorst <maarten.lankhorst at linux.intel.com> writes:

> 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?
>

intel_set_memory_cxsr() did the access with all the power wells off.

So perhaps my commit message was off. Don't know if crtc
state is what matters, but evidence points to the powers
being off regardless.

-Mika

> ~Maarten