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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx