diff mbox

drm/i915: Provide a modparam to disable firmware loading

Message ID 1460566637-19638-1-git-send-email-ben@bwidawsk.net (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky April 13, 2016, 4:57 p.m. UTC
For debug and development purposes only.

Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 13 +++++++++++++
 drivers/gpu/drm/i915/i915_gpu_error.c   |  3 +++
 drivers/gpu/drm/i915/i915_params.c      |  6 ++++++
 drivers/gpu/drm/i915/i915_params.h      |  1 +
 drivers/gpu/drm/i915/intel_csr.c        | 11 +++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 11 +++++++++++
 6 files changed, 45 insertions(+)

Comments

Dave Gordon April 13, 2016, 5:45 p.m. UTC | #1
On 13/04/16 17:57, Ben Widawsky wrote:
> For debug and development purposes only.
>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     | 13 +++++++++++++
>   drivers/gpu/drm/i915/i915_gpu_error.c   |  3 +++
>   drivers/gpu/drm/i915/i915_params.c      |  6 ++++++
>   drivers/gpu/drm/i915/i915_params.h      |  1 +
>   drivers/gpu/drm/i915/intel_csr.c        | 11 +++++++++++
>   drivers/gpu/drm/i915/intel_guc_loader.c | 11 +++++++++++
>   6 files changed, 45 insertions(+)

You can already disable GuC loading in the existing driver by turning 
off GuC submission, or if you have the previously-posted "add 
enable_guc_loading" patch, then you can separately control the loading 
of the GuC firmware and the use of the GuC for batch submission.

.Dave.
Ben Widawsky April 13, 2016, 5:48 p.m. UTC | #2
On Wed, Apr 13, 2016 at 06:45:32PM +0100, Dave Gordon wrote:
> On 13/04/16 17:57, Ben Widawsky wrote:
> > For debug and development purposes only.
> > 
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >   drivers/gpu/drm/i915/i915_debugfs.c     | 13 +++++++++++++
> >   drivers/gpu/drm/i915/i915_gpu_error.c   |  3 +++
> >   drivers/gpu/drm/i915/i915_params.c      |  6 ++++++
> >   drivers/gpu/drm/i915/i915_params.h      |  1 +
> >   drivers/gpu/drm/i915/intel_csr.c        | 11 +++++++++++
> >   drivers/gpu/drm/i915/intel_guc_loader.c | 11 +++++++++++
> >   6 files changed, 45 insertions(+)
> 
> You can already disable GuC loading in the existing driver by turning off
> GuC submission,

I saw this. I believe that behavior is incorrect but I didn't bother to fix it.
GuC submission and GuC loading are separate.

> or if you have the previously-posted "add enable_guc_loading" patch, then you
> can separately control the loading of the GuC firmware and the use of the GuC
> for batch submission.

Sounds to me like that patch should be merged, and we could roll that into this
and give me what I want.

> 
> .Dave.
Jani Nikula April 14, 2016, 8:27 a.m. UTC | #3
On Wed, 13 Apr 2016, Ben Widawsky <ben@bwidawsk.net> wrote:
> +module_param_named_unsafe(disable_firmware_loading, i915.disable_firmware_loading, uint, 0400);
> +MODULE_PARM_DESC(disable_firmware_loading,
> +	"Bypass loading of firmware based on mask. This overrides all other firmware module parameters. (0:all firmwares enabled (default), 1<<0:disable GuC, 1<<1:disable DMC");

I'd like all parameters like this to read "enable", defaulting to
enabled or disabled, possibly with platform specific defaults.

I know there's the underlying assumption that firmware loading is
enabled by default, therefore disabling is a special case, so you call
this disable. But we've already got enable_whatnot, with all kinds of
defaults. The disable_whatnot ones are confusing.

I can easily imagine adding platform specific defaults to
enable_firmware_loading, particularly during platform enabling.

BR,
Jani.
Ben Widawsky April 14, 2016, 3:07 p.m. UTC | #4
On Thu, Apr 14, 2016 at 11:27:25AM +0300, Jani Nikula wrote:
> On Wed, 13 Apr 2016, Ben Widawsky <ben@bwidawsk.net> wrote:
> > +module_param_named_unsafe(disable_firmware_loading, i915.disable_firmware_loading, uint, 0400);
> > +MODULE_PARM_DESC(disable_firmware_loading,
> > +	"Bypass loading of firmware based on mask. This overrides all other firmware module parameters. (0:all firmwares enabled (default), 1<<0:disable GuC, 1<<1:disable DMC");
> 
> I'd like all parameters like this to read "enable", defaulting to
> enabled or disabled, possibly with platform specific defaults.
> 
> I know there's the underlying assumption that firmware loading is
> enabled by default, therefore disabling is a special case, so you call
> this disable. But we've already got enable_whatnot, with all kinds of
> defaults. The disable_whatnot ones are confusing.
> 
> I can easily imagine adding platform specific defaults to
> enable_firmware_loading, particularly during platform enabling.
> 
> BR,
> Jani.

I'm happy to change it, but it'd be nice to have some kind of ack that someone
will merge it before I bother with that.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c0b28fa..75551b5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2445,6 +2445,9 @@  static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	u32 tmp, i;
 
+	if (i915.disable_firmware_loading & (1<<0))
+		return 0;
+
 	if (!HAS_GUC_UCODE(dev_priv))
 		return 0;
 
@@ -2519,6 +2522,11 @@  static int i915_guc_info(struct seq_file *m, void *data)
 	struct intel_engine_cs *engine;
 	u64 total = 0;
 
+	if (i915.disable_firmware_loading & (1<<0)) {
+		seq_printf(m, "GuC disabled by module parameter.\n");
+		return 0;
+	}
+
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
 
@@ -2784,6 +2792,11 @@  static int i915_dmc_info(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_csr *csr;
 
+	if (i915.disable_firmware_loading & (1<<1)) {
+		seq_puts(m, "disabled by module parameter\n");
+		return 0;
+	}
+
 	if (!HAS_CSR(dev)) {
 		seq_puts(m, "not supported\n");
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 8562866..543477e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -369,6 +369,9 @@  int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		   dev->pdev->subsystem_vendor,
 		   dev->pdev->subsystem_device);
 	err_printf(m, "IOMMU enabled?: %d\n", error->iommu);
+	err_printf(m, "Firmwares disabled: %s%s\n",
+		   i915.disable_firmware_loading & (1<<0) ? "GuC " : "",
+		   i915.disable_firmware_loading & (1<<1) ? "DMC " : "");
 
 	if (HAS_CSR(dev)) {
 		struct intel_csr *csr = &dev_priv->csr;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1779f02..6a73716 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -58,6 +58,7 @@  struct i915_params i915 __read_mostly = {
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
+	.disable_firmware_loading = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -207,6 +208,11 @@  MODULE_PARM_DESC(guc_log_level,
 module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
 MODULE_PARM_DESC(enable_dp_mst,
 	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
+
 module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
 MODULE_PARM_DESC(inject_load_failure,
 	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
+
+module_param_named_unsafe(disable_firmware_loading, i915.disable_firmware_loading, uint, 0400);
+MODULE_PARM_DESC(disable_firmware_loading,
+	"Bypass loading of firmware based on mask. This overrides all other firmware module parameters. (0:all firmwares enabled (default), 1<<0:disable GuC, 1<<1:disable DMC");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 02bc278..c1b6c0f 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -50,6 +50,7 @@  struct i915_params {
 	int mmio_debug;
 	int edp_vswing;
 	unsigned int inject_load_failure;
+	unsigned int disable_firmware_loading;
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
 	bool fastboot;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 3f57cb9..d6f553b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -246,6 +246,9 @@  void intel_csr_load_program(struct drm_i915_private *dev_priv)
 	u32 *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
+	if (i915.disable_firmware_loading & (1<<1))
+		return;
+
 	if (!IS_GEN9(dev_priv)) {
 		DRM_ERROR("No CSR support available for this platform\n");
 		return;
@@ -433,6 +436,11 @@  void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 
 	INIT_WORK(&dev_priv->csr.work, csr_load_work_fn);
 
+	if (i915.disable_firmware_loading & (1<<1)) {
+		DRM_INFO("CSR support disabled by module parameter\n");
+		return;
+	}
+
 	if (!HAS_CSR(dev_priv))
 		return;
 
@@ -465,6 +473,9 @@  void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
  */
 void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
 {
+	if (i915.disable_firmware_loading & (1<<1))
+		return;
+
 	if (!HAS_CSR(dev_priv))
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 876e5da..cdc2c8b 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -389,6 +389,9 @@  int intel_guc_ucode_load(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	int retries, err = 0;
 
+	if (i915.disable_firmware_loading & (1<<0))
+		return 0;
+
 	if (!i915.enable_guc_submission)
 		return 0;
 
@@ -631,6 +634,11 @@  void intel_guc_ucode_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (i915.disable_firmware_loading & (1<<0)) {
+		DRM_INFO("GuC support disabled by module parameter\n");
+		return;
+	}
+
 	if (!HAS_GUC_SCHED(dev))
 		i915.enable_guc_submission = false;
 
@@ -677,6 +685,9 @@  void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	if (i915.disable_firmware_loading & (1<<0))
+		return;
+
 	mutex_lock(&dev->struct_mutex);
 	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_disable(dev);