Re: [isg-gms] [RFC 5/6] drm/i915/vlv: Add optimal field in intel_crtc_wm_state

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Jun 08, 2016 at 05:22:43PM +0200, Chi Ding wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 

This patch needs a commit message.  Those of us actively working on
watermarks right now understand the end goal for two-stage watermarks,
but I don't think most other developers looking at this code (or
bisecting something to this patch) will immediately understand what
'optimal' means in the context of watermarks (or why we'd ever have
non-optimal watermarks).

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Chi Ding <chix.ding@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 23 ++++++++++++++---------
>  drivers/gpu/drm/i915/intel_pm.c  | 11 +++++++++--
>  2 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 31118e1..6d2e628 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -420,6 +420,16 @@ struct skl_pipe_wm {
>  	uint32_t linetime;
>  };
>  
> +struct vlv_wm_state {
> +	struct vlv_pipe_wm wm[3];
> +	struct vlv_sr_wm sr[3];
> +	uint16_t fifo_size[I915_MAX_PLANES];
> +	uint8_t num_active_planes;
> +	uint8_t num_levels;
> +	uint8_t level;
> +	bool cxsr;
> +};
> +
>  struct intel_crtc_wm_state {
>  	union {
>  		struct {
> @@ -440,6 +450,10 @@ struct intel_crtc_wm_state {
>  		} ilk;
>  
>  		struct {
> +			struct vlv_wm_state optimal;
> +		} vlv;
> +
> +		struct {
>  			/* gen9+ only needs 1-step wm programming */
>  			struct skl_pipe_wm optimal;
>  
> @@ -621,15 +635,6 @@ struct intel_crtc_state {
>  	uint32_t gamma_mode;
>  };
>  
> -struct vlv_wm_state {
> -	struct vlv_pipe_wm wm[3];
> -	struct vlv_sr_wm sr[3];
> -	uint16_t fifo_size[I915_MAX_PLANES];
> -	uint8_t num_active_planes;
> -	uint8_t num_levels;
> -	uint8_t level;
> -	bool cxsr;
> -};
>  
>  struct intel_crtc {
>  	struct drm_crtc base;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3fc80f9..502af9f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1093,7 +1093,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
> -	struct vlv_wm_state *wm_state = &crtc->wm_state;
> +	struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
>  	struct intel_plane *plane;
>  	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
>  	int level;
> @@ -1335,6 +1335,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  	struct vlv_wm_values wm = {};
>  
>  	vlv_compute_wm(intel_crtc->config);
> +	intel_crtc->wm_state = intel_crtc->config->wm.vlv.optimal;

Is this now the 'active' set of watermarks?  If so, we might want to
move it into crtc->wm.active.vlv to match the handling we have on other
platforms.

Also, it looks like VLV-style watermarks can still race here.  I think
we need to grab wm_mutex before setting wm_state/active like we do on
the other platforms so that the state doesn't get changed by one thread
while another one is in the middle of merging WM's into final results.

>  	vlv_merge_wm(dev, &wm);
>  
>  	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
> @@ -1377,6 +1378,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  		chv_set_memory_dvfs(dev_priv, true);
>  
>  	dev_priv->wm.vlv = wm;
> +

Stray whitespace?


Matt

>  }
>  
>  #define single_plane_enabled(mask) is_power_of_2(mask)
> @@ -4286,10 +4288,15 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  		mutex_unlock(&dev_priv->rps.hw_lock);
>  	}
>  
> -	for_each_pipe(dev_priv, pipe)
> +	for_each_intel_crtc(dev, crtc) {
> +		pipe = crtc->pipe;
> +		to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
> +			= crtc->wm_state;
> +
>  		DRM_DEBUG_KMS("Initial watermarks: pipe %c, plane=%d, cursor=%d, sprite0=%d, sprite1=%d\n",
>  			      pipe_name(pipe), wm->pipe[pipe].primary, wm->pipe[pipe].cursor,
>  			      wm->pipe[pipe].sprite[0], wm->pipe[pipe].sprite[1]);
> +	}
>  
>  	DRM_DEBUG_KMS("Initial watermarks: SR plane=%d, SR cursor=%d level=%d cxsr=%d\n",
>  		      wm->sr.plane, wm->sr.cursor, wm->level, wm->cxsr);
> -- 
> 1.8.0.1
> 
> -------------------------------------
> isg-gms@eclists.intel.com
> https://eclists.intel.com/sympa/info/isg-gms
> Unsubscribe by sending email to sympa@eclists.intel.com with subject "Unsubscribe isg-gms"

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx