[Intel-gfx] [PATCH 1/2] drm/i915: Use a modparam to restrict exposed engines
Summers, Stuart
stuart.summers at intel.com
Tue Oct 8 15:21:03 UTC 2019
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 <chris at chris-wilson.co.uk>
> Cc: Stuart Summers <stuart.summers at intel.com>
> Cc: Andi Shyti <andi.shyti at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Martin Peres <martin.peres at 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) \
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20191008/0a4c26b1/attachment.bin>
More information about the Intel-gfx
mailing list