From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 08/53] drm/i915/bdw: Macro for LRCs and module option for Execlists Date: Wed, 18 Jun 2014 22:19:00 +0200 Message-ID: <20140618201900.GY5821@phenom.ffwll.local> References: <1402673891-14618-1-git-send-email-oscar.mateo@intel.com> <1402673891-14618-9-git-send-email-oscar.mateo@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f49.google.com (mail-wg0-f49.google.com [74.125.82.49]) by gabe.freedesktop.org (Postfix) with ESMTP id EC4056E786 for ; Wed, 18 Jun 2014 13:19:07 -0700 (PDT) Received: by mail-wg0-f49.google.com with SMTP id y10so1386499wgg.20 for ; Wed, 18 Jun 2014 13:19:07 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402673891-14618-9-git-send-email-oscar.mateo@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: oscar.mateo@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 13, 2014 at 04:37:26PM +0100, oscar.mateo@intel.com wrote: > From: Oscar Mateo > > 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. In dev_priv, lrc_enabled will reflect the state of whether or > not we've actually properly initialized these new contexts. This helps > the transition in the code but is a candidate for removal at some point. > > 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. > > Signed-off-by: Ben Widawsky (v1) > Signed-off-by: Damien Lespiau (v3) > Signed-off-by: Oscar Mateo (v2 & v4) > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ > drivers/gpu/drm/i915/i915_params.c | 6 ++++++ > drivers/gpu/drm/i915/intel_lrc.c | 8 ++++++++ > 3 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ccc1ba6..dac0db1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1519,6 +1519,7 @@ struct drm_i915_private { > > uint32_t hw_context_size; > struct list_head context_list; > + bool lrc_enabled; > > u32 fdi_rx_config; > > @@ -1944,6 +1945,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) > @@ -2029,6 +2031,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; > @@ -2420,6 +2423,9 @@ struct intel_context * > i915_gem_context_validate(struct drm_device *dev, struct drm_file *file, > struct intel_engine_cs *ring, const u32 ctx_id); > > +/* intel_lrc.c */ > +bool intel_enable_execlists(struct drm_device *dev); > + > /* i915_gem_render_state.c */ > int i915_gem_render_state_init(struct intel_engine_cs *ring); > /* i915_gem_evict.c */ > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index d05a2af..b7455f8 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -37,6 +37,7 @@ struct i915_params i915 __read_mostly = { > .enable_fbc = -1, > .enable_hangcheck = true, > .enable_ppgtt = -1, > + .enable_execlists = -1, > .enable_psr = 0, > .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), > .disable_power_well = 1, > @@ -116,6 +117,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: false)"); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 49bb6fc..58cead1 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -40,3 +40,11 @@ > #include > #include > #include "i915_drv.h" > + > +bool intel_enable_execlists(struct drm_device *dev) > +{ > + if (!i915.enable_execlists) > + return false; > + > + return HAS_LOGICAL_RING_CONTEXTS(dev) && USES_PPGTT(dev); > +} Nitpick: Best practice nowadays for options with complicated details is to have a sanitized function called early in init. Code then just uses i915.foo without calling anything. And the parameter needs to be read-only, but that's already the case. See e.g. ppgtt handling. Of course if your code only uses this once then this is moot - I didn't read ahead. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch