Re: [PATCH] drm/i915: Drop ilk_wm_max_level()

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

On Mon, Dec 21, 2015 at 11:32:50AM -0800, Matt Roper wrote:
> Storing the max_level in dev_priv as VLV/CHV already do is a bit simpler
> than calling this standalone function, especially since some of the
> callsites need to special-case the call to check whether they're running
> on VLV/CHV.
> 
> This function was further confusing since it wasn't actually specific to
> ILK-style platforms as its name implied (it was also used on Skylake and
> would even be called on pre-ILK platforms via debugfs).  Aside from this
> function, our watermark programming across various platform-types
> doesn't really have any overlap.
> 
> Note that we do change debugfs behavior slightly with this patch;
> i915_*_wm_latency files will all now appear empty on pre-ILK platforms
> rather than showing two watermark levels of 0.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++-------------
>  drivers/gpu/drm/i915/intel_pm.c     | 44 +++++++++++++++++--------------------
>  2 files changed, 22 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0fc38bb..ae36656 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4445,14 +4445,7 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
>  {
>  	struct drm_device *dev = m->private;
>  	int level;
> -	int num_levels;
> -
> -	if (IS_CHERRYVIEW(dev))
> -		num_levels = 3;
> -	else if (IS_VALLEYVIEW(dev))
> -		num_levels = 1;
> -	else
> -		num_levels = ilk_wm_max_level(dev) + 1;
> +	int num_levels = to_i915(dev)->wm.max_level + 1;
>  
>  	drm_modeset_lock_all(dev);
>  
> @@ -4560,18 +4553,11 @@ static ssize_t wm_latency_write(struct file *file, const char __user *ubuf,
>  	struct seq_file *m = file->private_data;
>  	struct drm_device *dev = m->private;
>  	uint16_t new[8] = { 0 };
> -	int num_levels;
> +	int num_levels = to_i915(dev)->wm.max_level + 1;
>  	int level;
>  	int ret;
>  	char tmp[32];
>  
> -	if (IS_CHERRYVIEW(dev))
> -		num_levels = 3;
> -	else if (IS_VALLEYVIEW(dev))
> -		num_levels = 1;
> -	else
> -		num_levels = ilk_wm_max_level(dev) + 1;
> -

One idea I had at some point would be adjust dev_priv->wm.max_level based
on what the user writes to this file. Ie. it would allow the user to
enable/disable specific wm levels in a fairly nice way. In theory you
can do that already, but a bunch of stuff in the wm code might
scream at/not like eg. a latency value of 0. If we would adjust max_levels
appropritely we should be able to avoid those problems.

To do that we should probably keep the ilk_wm_max_level() (or something
like it) around and it would return the hardware limit, whereas dev_priv
contains the sw limit.

>  	if (len >= sizeof(tmp))
>  		return -EINVAL;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b076580..71d814c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2027,7 +2027,7 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[8])
>  	if (IS_GEN9(dev)) {
>  		uint32_t val;
>  		int ret, i;
> -		int level, max_level = ilk_wm_max_level(dev);
> +		int level, max_level = dev_priv->wm.max_level;
>  
>  		/* read the first set of memory latencies[0:3] */
>  		val = 0; /* data0 to be programmed to 0 for first set */
> @@ -2142,24 +2142,11 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5])
>  		wm[3] *= 2;
>  }
>  
> -int ilk_wm_max_level(const struct drm_device *dev)
> -{
> -	/* how many WM levels are we expecting */
> -	if (INTEL_INFO(dev)->gen >= 9)
> -		return 7;
> -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		return 4;
> -	else if (INTEL_INFO(dev)->gen >= 6)
> -		return 3;
> -	else
> -		return 2;
> -}
> -
>  static void intel_print_wm_latency(struct drm_device *dev,
>  				   const char *name,
>  				   const uint16_t wm[8])
>  {
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = to_i915(dev)->wm.max_level;
>  
>  	for (level = 0; level <= max_level; level++) {
>  		unsigned int latency = wm[level];
> @@ -2188,7 +2175,7 @@ static void intel_print_wm_latency(struct drm_device *dev,
>  static bool ilk_increase_wm_latency(struct drm_i915_private *dev_priv,
>  				    uint16_t wm[5], uint16_t min)
>  {
> -	int level, max_level = ilk_wm_max_level(dev_priv->dev);
> +	int level, max_level = dev_priv->wm.max_level;
>  
>  	if (wm[0] >= min)
>  		return false;
> @@ -2242,6 +2229,13 @@ static void ilk_setup_wm_latency(struct drm_device *dev)
>  
>  	if (IS_GEN6(dev))
>  		snb_wm_latency_quirk(dev);
> +
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		dev_priv->wm.max_level = 4;
> +	else if (INTEL_INFO(dev)->gen >= 6)
> +		dev_priv->wm.max_level = 3;
> +	else
> +		dev_priv->wm.max_level = 2;

Maybe we could just compute this based on how many non-zero wm latency
values we managed to read out?

>  }
>  
>  static void skl_setup_wm_latency(struct drm_device *dev)
> @@ -2250,6 +2244,8 @@ static void skl_setup_wm_latency(struct drm_device *dev)
>  
>  	intel_read_wm_latency(dev, dev_priv->wm.skl_latency);
>  	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
> +
> +	dev_priv->wm.max_level = 7;
>  }
>  
>  /* Compute new watermarks for the pipe */
> @@ -2265,7 +2261,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = dev_priv->wm.max_level;
>  	/* LP0 watermark maximums depend on this pipe alone */
>  	struct intel_wm_config config = {
>  		.num_pipes_active = 1,
> @@ -2389,7 +2385,7 @@ static void ilk_wm_merge(struct drm_device *dev,
>  			 struct intel_pipe_wm *merged)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = dev_priv->wm.max_level;
>  	int last_enabled_level = max_level;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/ single pipe */
> @@ -2530,7 +2526,7 @@ static struct intel_pipe_wm *ilk_find_best_result(struct drm_device *dev,
>  						  struct intel_pipe_wm *r1,
>  						  struct intel_pipe_wm *r2)
>  {
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = to_i915(dev)->wm.max_level;
>  	int level1 = 0, level2 = 0;
>  
>  	for (level = 1; level <= max_level; level++) {
> @@ -3232,7 +3228,7 @@ static void skl_compute_pipe_wm(struct intel_crtc_state *cstate,
>  {
>  	struct drm_device *dev = cstate->base.crtc->dev;
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = dev_priv->wm.max_level;
>  
>  	for (level = 0; level <= max_level; level++) {
>  		skl_compute_wm_level(dev_priv, ddb, cstate,
> @@ -3248,7 +3244,7 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  				   struct skl_wm_values *r,
>  				   struct intel_crtc *intel_crtc)
>  {
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = to_i915(dev)->wm.max_level;
>  	enum pipe pipe = intel_crtc->pipe;
>  	uint32_t temp;
>  	int i;
> @@ -3317,7 +3313,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	struct intel_crtc *crtc;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		int i, level, max_level = ilk_wm_max_level(dev);
> +		int i, level, max_level = dev_priv->wm.max_level;
>  		enum pipe pipe = crtc->pipe;
>  
>  		if (!new->dirty[pipe])
> @@ -3711,7 +3707,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  	int level, i, max_level;
>  	uint32_t temp;
>  
> -	max_level = ilk_wm_max_level(dev);
> +	max_level = dev_priv->wm.max_level;
>  
>  	hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
>  
> @@ -3801,7 +3797,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		active->wm[0].cur_val = tmp & WM0_PIPE_CURSOR_MASK;
>  		active->linetime = hw->wm_linetime[pipe];
>  	} else {
> -		int level, max_level = ilk_wm_max_level(dev);
> +		int level, max_level = dev_priv->wm.max_level;
>  
>  		/*
>  		 * For inactive pipes, all watermark levels
> -- 
> 2.1.4
> 
> _______________________________________________
> 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