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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

> 
> 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;
>  }
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx