diff mbox

[08/53] drm/i915/bdw: Macro for LRCs and module option for Execlists

Message ID 1402673891-14618-9-git-send-email-oscar.mateo@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

oscar.mateo@intel.com June 13, 2014, 3:37 p.m. UTC 
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. 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 <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)
---
 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(+)

Comments

Daniel Vetter June 18, 2014, 8:19 p.m. UTC | #1 
On Fri, Jun 13, 2014 at 04:37:26PM +0100, oscar.mateo@intel.com 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. 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 <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)
> ---
>  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 <drm/drmP.h>
>  #include <drm/i915_drm.h>
>  #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
oscar.mateo@intel.com June 19, 2014, 9:04 a.m. UTC | #2 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:19 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 08/53] drm/i915/bdw: Macro for LRCs and
> module option for Execlists
> 
> On Fri, Jun 13, 2014 at 04:37:26PM +0100, oscar.mateo@intel.com 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. 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 <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)
> > ---
> >  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 <drm/drmP.h>
> >  #include <drm/i915_drm.h>
> >  #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

Yes, I haven´t missed the 20+ True PPGTT early sanitize patches in the list :)
Ok, will do!

-- Oscar
diff mbox

Patch

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 <drm/drmP.h>
 #include <drm/i915_drm.h>
 #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);
+}