On Tue, 2019-10-01 at 14:54 +0100, Chris Wilson wrote: > Allow the user to restrict the available set of engines via a module > parameter. > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> > Cc: Stuart Summers <stuart.summ...@intel.com> > Cc: Andi Shyti <andi.sh...@intel.com> > Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com> > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com> > Cc: Martin Peres <martin.pe...@linux.intel.com> > --- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 35 ++++++++++++++++----- > -- > drivers/gpu/drm/i915/i915_gem.c | 5 ++++ > drivers/gpu/drm/i915/i915_params.c | 4 +++ > drivers/gpu/drm/i915/i915_params.h | 1 + > 4 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index 80fd072ac719..690da64ec256 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -389,6 +389,29 @@ void intel_engines_cleanup(struct > drm_i915_private *i915) > } > } > > +static bool engine_available(struct drm_i915_private *i915, int id) > +{ > + /* uAPI -- modparam bits must be consistent between kernels */ > + static const unsigned int param_bit[] = { > + [RCS0] = BIT(0), > + [VCS0] = BIT(1), > + [BCS0] = BIT(2), > + [VECS0] = BIT(3), > + [VCS1] = BIT(4), > + [VCS2] = BIT(5), > + [VCS3] = BIT(6), > + [VECS1] = BIT(7), > + };
To be a little controversial here... I don't see how this is better than just matching to our currently available engines for the platform. The user will still need to look in the code to determine what engines are available. I understand this restricts us to a static module parameter across kernel revisions, but at the same time, this will add a maintenance burden for an unsafe feature, something not used regularly. And it seems like this will be easy to get stale over time. > + > + if (!HAS_ENGINE(i915, id)) > + return false; > + > + if (!(i915_modparams.engines & param_bit[id])) > + return false; > + > + return true; > +} > + > /** > * intel_engines_init_mmio() - allocate and prepare the Engine > Command Streamers > * @i915: the i915 device > @@ -397,7 +420,6 @@ void intel_engines_cleanup(struct > drm_i915_private *i915) > */ > int intel_engines_init_mmio(struct drm_i915_private *i915) > { > - struct intel_device_info *device_info = > mkwrite_device_info(i915); > const unsigned int engine_mask = INTEL_INFO(i915)->engine_mask; > unsigned int mask = 0; > unsigned int i; > @@ -411,7 +433,7 @@ int intel_engines_init_mmio(struct > drm_i915_private *i915) > return -ENODEV; > > for (i = 0; i < ARRAY_SIZE(intel_engines); i++) { > - if (!HAS_ENGINE(i915, i)) > + if (!engine_available(i915, i)) > continue; > > err = intel_engine_setup(&i915->gt, i); > @@ -421,14 +443,7 @@ int intel_engines_init_mmio(struct > drm_i915_private *i915) > mask |= BIT(i); > } > > - /* > - * Catch failures to update intel_engines table when the new > engines > - * are added to the driver by a warning and disabling the > forgotten > - * engines. > - */ > - if (WARN_ON(mask != engine_mask)) > - device_info->engine_mask = mask; > - > + mkwrite_device_info(i915)->engine_mask = mask; > RUNTIME_INFO(i915)->num_engines = hweight32(mask); > > intel_gt_check_and_clear_faults(&i915->gt); > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 3d3fda4cae99..bb25731466a9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1308,6 +1308,11 @@ int i915_gem_init(struct drm_i915_private > *dev_priv) > { > int ret; > > + if (!RUNTIME_INFO(dev_priv)->num_engines) { > + intel_gt_set_wedged_on_init(&dev_priv->gt); > + return 0; > + } Wait, why do we want to force a wedge if no engines are present? This seems like an intentional limitation if we get to this point, so it doesn't seem necessary to me to force this. > + > /* We need to fallback to 4K pages if host doesn't support huge > gtt. */ > if (intel_vgpu_active(dev_priv) && > !intel_vgpu_has_huge_gtt(dev_priv)) > mkwrite_device_info(dev_priv)->page_sizes = > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 296452f9efe4..27813bd79aa8 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -44,6 +44,10 @@ i915_param_named(modeset, int, 0400, > "Use kernel modesetting [KMS] (0=disable, " > "1=on, -1=force vga console preference [default])"); > > +i915_param_named(engines, uint, 0400, > + "Only expose selected command streamers [GPU engines] > (0=disable GPU, " > + "-1/0xffffffff enable all [default]). Each bit corresponds to a > different phyiscal engine: 0=RCS0, 1=VCS0, 2=BCS0, 3=VECS0, 4=VCS1, > 5=VCS2, 6=VCS3, 7=VECS1"); Shouldn't this be unsafe? Thanks, Stuart > + > i915_param_named_unsafe(enable_dc, int, 0400, > "Enable power-saving display C-states. " > "(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6)"); > diff --git a/drivers/gpu/drm/i915/i915_params.h > b/drivers/gpu/drm/i915/i915_params.h > index d29ade3b7de6..f876db78a59a 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -45,6 +45,7 @@ struct drm_printer; > #define I915_PARAMS_FOR_EACH(param) \ > param(char *, vbt_firmware, NULL) \ > param(int, modeset, -1) \ > + param(unsigned int, engines, -1) \ > param(int, lvds_channel_mode, 0) \ > param(int, panel_use_ssc, -1) \ > param(int, vbt_sdvo_panel_type, -1) \
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx