Re: [PATCH 01/18] drm/i915: change no_fbc_reason from enum to string

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 20, 2015 at 11:49:47AM -0200, Paulo Zanoni wrote:
> I wanted to add yet another check to intel_fbc_update() and realized
> I would need to create yet another enum no_fbc_reason case. So I
> remembered this patch series that Damien wrote a long time ago and
> nobody ever reviewed, so I decided to reimplement it since the code
> changed a lot since then.
> 
> Credits-to: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah that enum indirection is pointless.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h     | 19 +--------
>  drivers/gpu/drm/i915/intel_drv.h    |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c    | 77 +++++++++----------------------------
>  4 files changed, 20 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a3b22bd..6ac4ba3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1640,7 +1640,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  		seq_puts(m, "FBC enabled\n");
>  	else
>  		seq_printf(m, "FBC disabled: %s\n",
> -			  intel_no_fbc_reason_str(dev_priv->fbc.no_fbc_reason));
> +			   dev_priv->fbc.no_fbc_reason);
>  
>  	if (INTEL_INFO(dev_priv)->gen >= 7)
>  		seq_printf(m, "Compressing: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8afda45..c334525 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -925,24 +925,7 @@ struct i915_fbc {
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> -	enum no_fbc_reason {
> -		FBC_OK, /* FBC is enabled */
> -		FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
> -		FBC_NO_OUTPUT, /* no outputs enabled to compress */
> -		FBC_STOLEN_TOO_SMALL, /* not enough space for buffers */
> -		FBC_UNSUPPORTED_MODE, /* interlace or doublescanned mode */
> -		FBC_MODE_TOO_LARGE, /* mode too large for compression */
> -		FBC_BAD_PLANE, /* fbc not supported on plane */
> -		FBC_NOT_TILED, /* buffer not tiled */
> -		FBC_MULTIPLE_PIPES, /* more than one pipe active */
> -		FBC_MODULE_PARAM,
> -		FBC_CHIP_DEFAULT, /* disabled by default on this chip */
> -		FBC_ROTATION, /* rotation is not supported */
> -		FBC_IN_DBG_MASTER, /* kernel debugger is active */
> -		FBC_BAD_STRIDE, /* stride is not supported */
> -		FBC_PIXEL_RATE, /* pixel rate is too big */
> -		FBC_PIXEL_FORMAT /* pixel format is invalid */
> -	} no_fbc_reason;
> +	const char *no_fbc_reason;
>  
>  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
>  	void (*enable_fbc)(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 27dccf3..dc55e4c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1287,7 +1287,6 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  			  enum fb_op_origin origin);
>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
> -const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
>  
>  /* intel_hdmi.c */
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index cf47352..d9d7e54 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -471,55 +471,14 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
> -{
> -	switch (reason) {
> -	case FBC_OK:
> -		return "FBC enabled but currently disabled in hardware";
> -	case FBC_UNSUPPORTED:
> -		return "unsupported by this chipset";
> -	case FBC_NO_OUTPUT:
> -		return "no output";
> -	case FBC_STOLEN_TOO_SMALL:
> -		return "not enough stolen memory";
> -	case FBC_UNSUPPORTED_MODE:
> -		return "mode incompatible with compression";
> -	case FBC_MODE_TOO_LARGE:
> -		return "mode too large for compression";
> -	case FBC_BAD_PLANE:
> -		return "FBC unsupported on plane";
> -	case FBC_NOT_TILED:
> -		return "framebuffer not tiled or fenced";
> -	case FBC_MULTIPLE_PIPES:
> -		return "more than one pipe active";
> -	case FBC_MODULE_PARAM:
> -		return "disabled per module param";
> -	case FBC_CHIP_DEFAULT:
> -		return "disabled per chip default";
> -	case FBC_ROTATION:
> -		return "rotation unsupported";
> -	case FBC_IN_DBG_MASTER:
> -		return "Kernel debugger is active";
> -	case FBC_BAD_STRIDE:
> -		return "framebuffer stride not supported";
> -	case FBC_PIXEL_RATE:
> -		return "pixel rate is too big";
> -	case FBC_PIXEL_FORMAT:
> -		return "pixel format is invalid";
> -	default:
> -		MISSING_CASE(reason);
> -		return "unknown reason";
> -	}
> -}
> -
>  static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
> -			      enum no_fbc_reason reason)
> +			      const char *reason)
>  {
>  	if (dev_priv->fbc.no_fbc_reason == reason)
>  		return;
>  
>  	dev_priv->fbc.no_fbc_reason = reason;
> -	DRM_DEBUG_KMS("Disabling FBC: %s\n", intel_no_fbc_reason_str(reason));
> +	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
>  }
>  
>  static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
> @@ -862,12 +821,12 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		i915.enable_fbc = 0;
>  
>  	if (i915.enable_fbc < 0) {
> -		set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT);
> +		set_no_fbc_reason(dev_priv, "disabled per chip default");
>  		goto out_disable;
>  	}
>  
>  	if (!i915.enable_fbc) {
> -		set_no_fbc_reason(dev_priv, FBC_MODULE_PARAM);
> +		set_no_fbc_reason(dev_priv, "disabled per module param");
>  		goto out_disable;
>  	}
>  
> @@ -882,12 +841,12 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  	 */
>  	crtc = intel_fbc_find_crtc(dev_priv);
>  	if (!crtc) {
> -		set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT);
> +		set_no_fbc_reason(dev_priv, "no output");
>  		goto out_disable;
>  	}
>  
>  	if (!multiple_pipes_ok(dev_priv)) {
> -		set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES);
> +		set_no_fbc_reason(dev_priv, "more than one pipe active");
>  		goto out_disable;
>  	}
>  
> @@ -898,18 +857,18 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  
>  	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
>  	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
> -		set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE);
> +		set_no_fbc_reason(dev_priv, "incompatible mode");
>  		goto out_disable;
>  	}
>  
>  	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
> -		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
> +		set_no_fbc_reason(dev_priv, "mode too large for compression");
>  		goto out_disable;
>  	}
>  
>  	if ((INTEL_INFO(dev_priv)->gen < 4 || HAS_DDI(dev_priv)) &&
>  	    intel_crtc->plane != PLANE_A) {
> -		set_no_fbc_reason(dev_priv, FBC_BAD_PLANE);
> +		set_no_fbc_reason(dev_priv, "FBC unsupported on plane");
>  		goto out_disable;
>  	}
>  
> @@ -918,28 +877,28 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  	 */
>  	if (obj->tiling_mode != I915_TILING_X ||
>  	    obj->fence_reg == I915_FENCE_REG_NONE) {
> -		set_no_fbc_reason(dev_priv, FBC_NOT_TILED);
> +		set_no_fbc_reason(dev_priv, "framebuffer not tiled or fenced");
>  		goto out_disable;
>  	}
>  	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
>  	    crtc->primary->state->rotation != BIT(DRM_ROTATE_0)) {
> -		set_no_fbc_reason(dev_priv, FBC_ROTATION);
> +		set_no_fbc_reason(dev_priv, "rotation unsupported");
>  		goto out_disable;
>  	}
>  
>  	if (!stride_is_valid(dev_priv, fb->pitches[0])) {
> -		set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE);
> +		set_no_fbc_reason(dev_priv, "framebuffer stride not supported");
>  		goto out_disable;
>  	}
>  
>  	if (!pixel_format_is_valid(fb)) {
> -		set_no_fbc_reason(dev_priv, FBC_PIXEL_FORMAT);
> +		set_no_fbc_reason(dev_priv, "pixel format is invalid");
>  		goto out_disable;
>  	}
>  
>  	/* If the kernel debugger is active, always disable compression */
>  	if (in_dbg_master()) {
> -		set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
> +		set_no_fbc_reason(dev_priv, "Kernel debugger is active");
>  		goto out_disable;
>  	}
>  
> @@ -947,12 +906,12 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
>  	    ilk_pipe_pixel_rate(intel_crtc->config) >=
>  	    dev_priv->cdclk_freq * 95 / 100) {
> -		set_no_fbc_reason(dev_priv, FBC_PIXEL_RATE);
> +		set_no_fbc_reason(dev_priv, "pixel rate is too big");
>  		goto out_disable;
>  	}
>  
>  	if (intel_fbc_setup_cfb(intel_crtc)) {
> -		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
> +		set_no_fbc_reason(dev_priv, "not enough stolen memory");
>  		goto out_disable;
>  	}
>  
> @@ -995,7 +954,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  	}
>  
>  	intel_fbc_schedule_enable(intel_crtc);
> -	dev_priv->fbc.no_fbc_reason = FBC_OK;
> +	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)\n";
>  	return;
>  
>  out_disable:
> @@ -1088,7 +1047,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  
>  	if (!HAS_FBC(dev_priv)) {
>  		dev_priv->fbc.enabled = false;
> -		dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
> +		dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
>  		return;
>  	}
>  
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx