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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
---
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?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
