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

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:19 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at 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 at intel.com wrote:
> > From: Oscar Mateo <oscar.mateo at 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 at bwidawsk.net> (v1)
> > Signed-off-by: Damien Lespiau <damien.lespiau at intel.com> (v3)
> > Signed-off-by: Oscar Mateo <oscar.mateo at 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