Re: [PATCH] drm/i915: Wait until after wm optimization to drop runtime PM reference

[Imre Deak <imre.deak@intel.com>]

Hi Matt,

On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote:
> At the end of an atomic commit, we currently wait for vblanks to
> complete, call put() on the various runtime PM references, and then
> try
> to optimize our watermarks (on platforms that need two-step watermark
> programming).  This can lead to watermark registers being programmed
> while the power well is powered down.  We need to wait until after
> watermark optimization is complete before dropping our runtime power
> references.
> 
> Note that in the future the watermark optimization is probably going
> to
> move to an asynchronous workqueue task that happens at some arbitrary
> point after vblank.  When we make that change, we'll no longer
> necessarily be operating under the power reference held here, so
> we'll
> need to wrap the watermark register programmin in a call to
> intel_runtime_pm_get_if_in_use() or similar.
> 
> Cc: arun.siluvery@linux.intel.com
> Cc: ville.syrjala@linux.intel.com
> Cc: maarten.lankhorst@linux.intel.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349
> Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark
> programming (v11)")
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 62d36a7..0af08d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	if (!state->legacy_cursor_update)
>  		intel_atomic_wait_for_vblanks(dev, dev_priv,
> crtc_vblank_mask);
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		intel_post_plane_update(to_intel_crtc(crtc));
> -
> -		if (put_domains[i])
> -			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> -	}
> -
> -	if (intel_state->modeset)
> -		intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
> -
>  	/*
>  	 * Now that the vblank has passed, we can go ahead and
> program the
>  	 * optimal watermarks on platforms that need two-step
> watermark
> @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  			dev_priv-
> >display.optimize_watermarks(intel_cstate);
>  	}
>  
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_post_plane_update(to_intel_crtc(crtc));
> +
> +		if (put_domains[i])
> +			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> +	}
> +
> +	if (intel_state->modeset)
> +> 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> +

Since the error was caused by writing some WM register while the HW is
suspended, I don't see what would be the point of programming it if the
register loses its value anyway after dropping the power reference.
What would make sense to me is to avoid programming the WM registers if
all the outputs are disabled (like they were when the bug triggered)
and program them next time around when any output gets enabled.

--Imre


>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx