[Intel-gfx] [PATCH 6/8] drm/i915/gen9+: Use the watermarks from crtc_state for everything.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:19PM +0200, Maarten Lankhorst wrote:
> There's no need to keep a duplicate skl_pipe_wm around any more,
> everything can be discovered from crtc_state, which we pass around
> correctly now even in case of plane disable.

You might want to add some clarification that
intel(crtc->state)->wm.skl.optimal and intel_crtc->wm.active always hold
the same value by the time we finally drop our CRTC locks, so there's no
need for the duplication on gen9.  The reason we have
intel_crtc->wm.active in general is because the two-step platforms
(ILK-style, VLV-style) need cross-CRTC information during watermark
updates (which are potentially racing if multiple CRTC's are updated
independently but simultaneously) and the intel_crtc data is protected
by a separate wm_mutex.  Watermark calculations triggered by one CRTC's
update need to know which values are actually active on the hardware
(old, intermediate, or final) while another CRTC update is still being
processed.  The types of CRTC updates that are allowed to race on gen9
don't have the same kind of inter-CRTC data dependency.

I think this patch needs some slight rebasing to apply cleanly on top of
Lyude's changes that landed yesterday, but if you expand the commit
message justification a bit,

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


Matt

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_pm.c      | 11 +++++------
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 23d8c72dade3..340861826c46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  		return;
>  
>  	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> -	sw_wm = &intel_crtc->wm.active.skl;
> +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
>  
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a176e6cebab3..9f04e26c4365 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -723,7 +723,6 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		union {
>  			struct intel_pipe_wm ilk;
> -			struct skl_pipe_wm skl;
>  		} active;
>  
>  		/* allow CxSR on this pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 05ccd253fd7a..be3dd8cdc7ae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  			      struct skl_ddb_allocation *ddb, /* out */
>  			      struct skl_pipe_wm *pipe_wm, /* out */
> +			      const struct skl_pipe_wm *old_pipe_wm,
>  			      bool *changed /* out */)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
>  	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
>  	int ret;
>  
> @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  	if (ret)
>  		return ret;
>  
> -	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> +	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
>  		*changed = false;
>  	else
>  		*changed = true;
> @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *intel_cstate =
>  			to_intel_crtc_state(cstate);
> +		const struct skl_pipe_wm *old_pipe_wm =
> +			&to_intel_crtc_state(crtc->state)->wm.skl.optimal;
>  
>  		pipe_wm = &intel_cstate->wm.skl.optimal;
>  		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
> -					 &changed);
> +					 old_pipe_wm, &changed);
>  		if (ret)
>  			return ret;
>  
> @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
>  
> -	intel_crtc->wm.active.skl = *pipe_wm;
> -
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
>  	/*
> @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device *dev)
>  		cstate = to_intel_crtc_state(crtc->state);
>  
>  		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
> -		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
>  
>  		if (!intel_crtc->active)
>  			hw->dirty_pipes |= drm_crtc_mask(crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795