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

[Matt Roper]

On Wed, Mar 04, 2015 at 05:26:36PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/04/2015 02:15 AM, 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).
> >
> >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;
> 
> Struggling to paint the whole picture here..
> 
> Why it was correct to replace primary->fb with primary->state->fb
> elsewhere, but not here?
> 
> Does the commit message mean there can be an active crtc with an
> active plane, but crtc->primary->fb == NULL so the wm recompute
> incorrectly configures for disabled plane?

Back when this code was first written, we didn't have universal planes
yet and the fb pointer was directly in the CRTC.   So at that time,
(crtc->fb == NULL) meant that the entire CRTC was off.

When I added universal plane support, I used Coccinelle to globally do a
s/crtc->fb/crtc->primary->fb/, so that change didn't really have any
special thought going into it.  However at that point we started
allowing you to have an explicitly disabled primary plane with an active
CRTC (something that had never been possible before with the legacy
API's).  In this case, crtc->primary->fb = NULL, but the CRTC is still
running.

So only considering a CRTC to be active if its primary plane has a
framebuffer isn't really correct in the universal plane and atomic
world.  Whether we use primary->fb or primary->state->fb doesn't really
matter in this case.

My understanding is that the reason we also checked fb and mode in
addition to intel_crtc->active had to do with quickboot logic and the
driver's ability to inherit state setup by the boot firmware.  I think
Jesse or Ville might be able to explain that part better and describe
any quickboot problems that this change might cause.

This patch is still sort of a bandaid; ultimately our watermark code
should all be state-based and we'll be looking at crtc->state->active
and plane->state->FOO and such to figure out how/when to update our
watermarks.  But the watermark rework isn't done yet and we're also sort
of in a weird limbo stage where planes are pretty much converted to
atomic, but CRTC's are still being worked on.


Matt

> 
> Regards,
> 
> Tvrtko

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795