Re: [PATCH 4/5] drm/i915: Use crtc->state->active in ilk/skl watermark calculations

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

On Sun, Mar 08, 2015 at 02:00:44PM -0700, Matt Roper wrote:
> Existing watermark code calls intel_crtc_active() to determine whether a CRTC
> is active for the purpose of watermark calculations (and bails out early if it
> determines the CRTC is not active).  However intel_crtc_active() only returns
> true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
> age of universal planes and atomic modeset since userspace can now disable the
> primary plane, but leave the CRTC (and other planes) running.
> 
> 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() returns
> false due to the disabled primary plane.
> 
> Switch the ILK and SKL watermark code over to use crtc->state->active rather
> than calling intel_crtc_active() so that we'll properly compute watermarks when
> re-enabling the primary plane.
> 
> Note that this commit doesn't touch callsites in the watermark code for
> older platforms since there were concerns that doing so would lead to
> other types of breakage.
> 
> Also note that all of the watermark calculation at the moment takes place after
> new crtc/plane states are swapped into the DRM objects.  This will change in
> the future, so we'll be working with in-flight state objects, but for the time
> being, crtc->state is what we want to operate on.
> 
> v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
>     ILK/SKL callsites with direct tests of crtc->state->active.  There is
>     concern that messing with intel_crtc_active() will lead to other breakage for
>     old hardware platforms.  (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>


If you make this use intel_crtc->active you can slap on my 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The intel_crtc->active to crtc->state->active change is more
controversial so I think we need to look at the code in more detail
before we go make such a big change.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d9b115e..dafd7de 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  	 * FIXME the plane might have an fb
>  	 * but be invisible (eg. due to clipping)
>  	 */
> -	if (!intel_crtc->base.state->active || !plane->state->fb)
> +	if (!crtc->state->active || !plane->state->fb)
>  		return 0;
>  
>  	if (WARN(clock == 0, "Pixel clock is zero!\n"))
> @@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  	struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
>  	u32 linetime, ips_linetime;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return 0;
>  
>  	/* The WM are computed with base on how long it takes to fill a single
> @@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_plane *plane;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return;
>  
>  	p->active = true;
> @@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  
>  	nth_active_pipe = 0;
>  	for_each_crtc(dev, crtc) {
> -		if (!intel_crtc_active(crtc))
> +		if (!crtc->state->active)
>  			continue;
>  
>  		if (crtc == for_crtc)
> @@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
>  	struct drm_plane *plane;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		config->num_pipes_active += intel_crtc_active(crtc);
> +		config->num_pipes_active += crtc->state->active;
>  
>  	/* FIXME: I don't think we need those two global parameters on SKL */
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> @@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	struct drm_framebuffer *fb;
>  	int i = 1; /* Index for sprite planes start */
>  
> -	p->active = intel_crtc_active(crtc);
> +	p->active = crtc->state->active;
>  	if (p->active) {
>  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> @@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  static uint32_t
>  skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
>  {
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return 0;
>  
>  	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
> @@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  		if (this_crtc->pipe == intel_crtc->pipe)
>  			continue;
>  
> -		if (!intel_crtc->base.state->active)
> +		if (!crtc->state->active)
>  			continue;
>  
>  		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
> @@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
>  	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return;
>  
>  	hw->dirty[pipe] = true;
> @@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
>  
> -	active->pipe_enabled = intel_crtc_active(crtc);
> +	active->pipe_enabled = crtc->state->active;
>  
>  	if (active->pipe_enabled) {
>  		u32 tmp = hw->wm_pipe[pipe];
> -- 
> 1.8.5.1

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