[Intel-gfx] [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Daniel Vetter]

On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote:
> On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote:
> > Universal planes allow us to have an active CRTC without a primary plane
> > framebuffer bound.  Drop the test for primary->fb from
> > intel_crtc_active() since we can clearly have active CRTC's without a
> > framebuffer, and this check is now interfering with watermark
> > calculations when we try to re-enable the primary plane.
> > 
> > Note that commit
> > 
> >         commit 0fda65680e92545caea5be7805a7f0a617fb6c20
> >         Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >         Date:   Fri Feb 27 15:12:35 2015 +0000
> > 
> >             drm/i915/skl: Update watermarks for Y tiling
> > 
> > adds a test for primary plane enable/disable to trigger a watermark
> > update (previously we ignored updates to primary planes, which wasn't
> > really correct, but we got lucky since we always pretended the primary
> > plane was on).  Tvrtko's patch tries to update watermarks when we
> > re-enable the primary plane, but that watermark computation gets aborted
> > early because intel_crtc_active() always returns false when the primary
> > plane is disabled; this leads to watermark underruns (at least on
> > platforms with ILK-style watermark code).
> 
> I think this will make a bunch of the old platforms blow up. Well, I
> think they might already blow up since they now look at crtc->state->fb
> based on the result of intel_crtc_active(). So I believe more fixes are
> needed all over the wm code at least.
> 
> In fact looking at the code, I think most (or all?) of the call sites in
> the in the ilk+ wm code should just be using crtc->active. So for some
> extra safety it might make sense to do leave intel_crtc_active() alone
> for now, or in fact we should maybe change it to also look at
> primary->state->fb instead. That should more or less keep the old
> platforms in the same state as they were before, while letting new
> platforms handle each plane properly.

intel_crtc->active shouldn't be used anywhere in state computation code.
The only thing you're allowed to look at is crtc_state->enable in the
atomic world.

Otherwise resource allocation isn't properly done when the pipe is in dpms
off state and then stuff blows up when userspace tries to dpms on. Which
is a big no-no, at least for proper backwards compat.
-Daniel

> 
> > 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 589addf..92cb2ff 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc)
> >  	 *
> >  	 * We can ditch the adjusted_mode.crtc_clock check as soon
> >  	 * as Haswell has gained clock readout/fastboot support.
> > -	 *
> > -	 * We can ditch the crtc->primary->fb check as soon as we can
> > -	 * properly reconstruct framebuffers.
> >  	 */
> > -	return intel_crtc->active && crtc->primary->fb &&
> > +	return intel_crtc->active &&
> >  		intel_crtc->config->base.adjusted_mode.crtc_clock;
> >  }
> >  
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch