Op 20-01-16 om 12:11 schreef Ville Syrjälä: > On Wed, Jan 20, 2016 at 08:18:03AM +0100, Maarten Lankhorst wrote: >> 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@xxxxxxxxxxxxxxx> >>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >>>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> >>>>>> --- >>>>>> 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); > Hmm. Yeah, I suppose that could work. I fear thinking about it is going > to give me a headache. Would really need nice two-stage wm programming > for gmch platforms as well to handle it properly. > Indeed, that would help a lot. Hopefully the ilk two-stage wm patch gets reapplied soon so we could convert gmch as well after.. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
