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

[Daniel Vetter]

On Thu, Mar 05, 2015 at 03:07:52PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/04/2015 05:42 PM, Matt Roper wrote:
> >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.
> 
> Yeah.. well, I don't see that anything will go bad with this, but this is
> with like 50% confidence.
> 
> And the comment said check can be removed when we can "properly reconstruct
> the framebuffer" anyway.
> 
> But what does "properly" mean there? Firmware take over looks there to me.
> So maybe it could really had been removed even regardless of this fallout?

Comment is outdated, what we want in the end is to just disable the
primary plane and make sure the hw can cope. Since if someone boots in vga
mode there is no primary plane (the vga plane is some entirely separate
beast).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch