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

[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.

-- 
Ville Syrjälä
Intel OTC