Re: [PATCH 14/14] drm/i915: Add two-stage ILK-style watermark programming (v4)

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

On Thu, Sep 17, 2015 at 02:38:12PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> It's worh nothing this series may have a soft dependency on Ville's cleanups,
> because else pixel_rate == 0 during boot. :(
> 
> This patch breaks when the initial sprite watermarks are set on ironlake, but no sprite is enabled.
> 
> A naive fix is below, without it my laptop won't power the screen because intermediate wm calculation fails. :(
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 41a4dbaf600b..bf874ff97024 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2457,7 +2457,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  
>  		a_wm->enable &= b_wm->enable;
>  		a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
> -		a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
> +		a_wm->spr_val = a->sprites_enabled ? max(a_wm->spr_val, b_wm->spr_val) : 0;
>  		a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
>  		a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
>  	}

Hmm. That could work, but feels a bit wrong. Would be nice if we could
always keep the watermarks totally consistent with the plane
configuration.

One thing we could do during init is simply calculate the optimal
watermarks for the current config and just blast them in. Then when
the first plane update/modeset happens the current state would
actually be sane wrt. the current plane config.

> 
> 
> Op 15-09-15 om 04:19 schreef Matt Roper:
> > <snip>
> > @@ -13202,6 +13229,20 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	/* FIXME: add subpixel order */
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, state);
> > +
> > +	/*
> > +	 * Now that the vblank has passed, we can go ahead and program the
> > +	 * optimal watermarks on platforms that need two-step watermark
> > +	 * programming.
> > +	 *
> > +	 * TODO: Move this (and other cleanup) to an async worker eventually.
> > +	 */
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		intel_cstate = to_intel_crtc_state(crtc->state);
> > +
> > +		dev_priv->display.optimize_watermarks(intel_cstate);
> > +	}
> ^Breaks on my skylake with a null pointer deref; I think you might want to hide it behind a if. :-)
> 
> _______________________________________________
> 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