Re: [PATCH 03/43] drm/i915/bdw: Macro for LRCs and module option for Execlists

Daniel Vetter
2014-08-11T13:57:15Z

On Thu, Jul 24, 2014 at 05:04:11PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
>
> GEN8 brings an expansion of the HW contexts: "Logical Ring Contexts".
> These expanded contexts enable a number of new abilities, especially
> "Execlists".
>
> The macro is defined to off until we have things in place to hope to
> work.
>
> v2: Rename "advanced contexts" to the more correct "logical ring
> contexts".
>
> v3: Add a module parameter to enable execlists. Execlist are relatively
> new, and so it'd be wise to be able to switch back to ring submission
> to debug subtle problems that will inevitably arise.
>
> v4: Add an intel_enable_execlists function.
>
> v5: Sanitize early, as suggested by Daniel. Remove lrc_enabled.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> (v1)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> (v3)
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> (v2, v4 & v5)
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/i915_gem.c | 3 +++
> drivers/gpu/drm/i915/i915_params.c | 6 ++++++
> drivers/gpu/drm/i915/intel_lrc.c | 11 +++++++++++
> drivers/gpu/drm/i915/intel_lrc.h | 3 +++
> 5 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 54c2bd9..a793d6d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2037,6 +2037,7 @@ struct drm_i915_cmd_table {
> #define I915_NEED_GFX_HWS(dev) (INTEL_INFO(dev)->need_gfx_hws)
>
> #define HAS_HW_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 6)
> +#define HAS_LOGICAL_RING_CONTEXTS(dev) 0
> #define HAS_ALIASING_PPGTT(dev) (INTEL_INFO(dev)->gen >= 6)
> #define HAS_PPGTT(dev) (INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
> #define USES_PPGTT(dev) intel_enable_ppgtt(dev, false)
> @@ -2122,6 +2123,7 @@ struct i915_params {
> int enable_rc6;
> int enable_fbc;
> int enable_ppgtt;
> + int enable_execlists;
> int enable_psr;
> unsigned int preliminary_hw_support;
> int disable_power_well;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e5d4d73..d8bf4fa 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4746,6 +4746,9 @@ int i915_gem_init(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
>
> + i915.enable_execlists = intel_sanitize_enable_execlists(dev,
> + i915.enable_execlists);

The ordering constraint on i915.enabel_ppgtt sanitization is imo
super-fragile. You'll all get a big price (and grumpy maintainer) if this
blows up. I'll add a patch on top to WARN about this.
-Daniel

> +
> mutex_lock(&dev->struct_mutex);
>
> if (IS_VALLEYVIEW(dev)) {
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index bbdee21..7f0fb72 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -35,6 +35,7 @@ struct i915_params i915 __read_mostly = {
> .vbt_sdvo_panel_type = -1,
> .enable_rc6 = -1,
> .enable_fbc = -1,
> + .enable_execlists = -1,
> .enable_hangcheck = true,
> .enable_ppgtt = -1,
> .enable_psr = 1,
> @@ -117,6 +118,11 @@ MODULE_PARM_DESC(enable_ppgtt,
> "Override PPGTT usage. "
> "(-1=auto [default], 0=disabled, 1=aliasing, 2=full)");
>
> +module_param_named(enable_execlists, i915.enable_execlists, int, 0400);
> +MODULE_PARM_DESC(enable_execlists,
> + "Override execlists usage. "
> + "(-1=auto [default], 0=disabled, 1=enabled)");
> +
> module_param_named(enable_psr, i915.enable_psr, int, 0600);
> MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 49bb6fc..21f7f1c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -40,3 +40,14 @@
> #include <drm/drmP.h>
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> +
> +int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists)
> +{
> + if (enable_execlists == 0)
> + return 0;
> +
> + if (HAS_LOGICAL_RING_CONTEXTS(dev) && USES_PPGTT(dev))
> + return 1;
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index f6830a4..75ee9c3 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -24,4 +24,7 @@
> #ifndef _INTEL_LRC_H_
> #define _INTEL_LRC_H_
>
> +/* Execlists */
> +int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
> +
> #endif /* _INTEL_LRC_H_ */
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch