| Message ID | 1440588497-13954-8-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: > From: Daniel Vetter <daniel.vetter@intel.com> > > If we really want to we can be more verbose here, but we really don't > need an entire function for this. > > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Sunil Kamath <sunil.kamath@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 20 -------------------- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_csr.c | 8 +++----- > 3 files changed, 3 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 1ddf3a9..3cdd319 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -537,26 +537,6 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > return true; > } > > -void i915_firmware_load_error_print(const char *fw_path, int err) > -{ > - DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err); > - > - /* > - * If the reason is not known assume -ENOENT since that's the most > - * usual failure mode. > - */ > - if (!err) > - err = -ENOENT; > - > - if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT)) > - return; > - > - DRM_ERROR( > - "The driver is built-in, so to load the firmware you need to\n" > - "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n" > - "in your initrd/initramfs image.\n"); > -} > - The point here was to clarify the reason why the loading failed, since that caused quite a confusion. It was a separate function since the same could've been called from the GuC loader too. I think the error message would be still useful. > static void intel_suspend_encoders(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 9f01c72..6241b49 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2654,7 +2654,6 @@ extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv); > extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv); > extern void i915_update_gfx_val(struct drm_i915_private *dev_priv); > int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on); > -void i915_firmware_load_error_print(const char *fw_path, int err); > > /* intel_hotplug.c */ > void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask); > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 00e9fbf..9d4b37b 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -237,10 +237,8 @@ static void finish_csr_load(const struct firmware *fw, void *context) > uint32_t *dmc_payload; > bool fw_loaded = false; > > - if (!fw) { > - i915_firmware_load_error_print(csr->fw_path, 0); > + if (!fw) > goto out; > - } > > if ((stepping == -ENODATA) || (substepping == -ENODATA)) { > DRM_ERROR("Unknown stepping info, firmware loading failed\n"); > @@ -340,6 +338,8 @@ static void finish_csr_load(const struct firmware *fw, void *context) > out: > if (fw_loaded || IS_BROXTON(dev)) > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > + else > + DRM_ERROR("Failed to load DMC firmware, disabling rpm\n"); > > release_firmware(fw); > } > @@ -380,8 +380,6 @@ void intel_csr_ucode_init(struct drm_device *dev) > &dev_priv->dev->pdev->dev, > GFP_KERNEL, dev_priv, > finish_csr_load); > - if (ret) > - i915_firmware_load_error_print(csr->fw_path, ret); > } > > /**
On 30/09/15 07:28, Imre Deak wrote: > On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: >> >> -void i915_firmware_load_error_print(const char *fw_path, int err) >> -{ >> - DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err); >> - >> - /* >> - * If the reason is not known assume -ENOENT since that's the most >> - * usual failure mode. >> - */ >> - if (!err) >> - err = -ENOENT; >> - >> - if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT)) >> - return; >> - >> - DRM_ERROR( >> - "The driver is built-in, so to load the firmware you need to\n" >> - "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n" >> - "in your initrd/initramfs image.\n"); >> -} >> - > > The point here was to clarify the reason why the loading failed, since > that caused quite a confusion. It was a separate function since the same > could've been called from the GuC loader too. I think the error message > would be still useful. Agreed 100%. The code of this function was a bit confusing, however this error message has proved very useful "on the field" many times already. Please preserve the message.
On Tue, Oct 06, 2015 at 12:38:00PM -0700, Marc Herbert wrote: > On 30/09/15 07:28, Imre Deak wrote: > >On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: > >> > >>-void i915_firmware_load_error_print(const char *fw_path, int err) > >>-{ > >>- DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err); > >>- > >>- /* > >>- * If the reason is not known assume -ENOENT since that's the most > >>- * usual failure mode. > >>- */ > >>- if (!err) > >>- err = -ENOENT; > >>- > >>- if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT)) > >>- return; > >>- > >>- DRM_ERROR( > >>- "The driver is built-in, so to load the firmware you need to\n" > >>- "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n" > >>- "in your initrd/initramfs image.\n"); > >>-} > >>- > > > >The point here was to clarify the reason why the loading failed, since > >that caused quite a confusion. It was a separate function since the same > >could've been called from the GuC loader too. I think the error message > >would be still useful. > > Agreed 100%. The code of this function was a bit confusing, however this > error message has proved very useful "on the field" many times already. > Please preserve the message. I dunno really, this is pretty much known by all the other gpu drivers. If you load things before the firmware is there it'll fail. Only difference is that with i915 dmc is the first time we require firmware images for some features. We should document this in the release notes for the skl/bxt+ stacks, but this really isn't all that useful in the kernel imo. It's really just how fw loading works (with drm drivers at least), and at least distros do have that operational knowledge since years. Also only printing something for the built-in case seems wrong, since you can equally screw things up when it's the driver is module. What we should have instead is just a DRM_INFO or similar that we're disabling a few pm features because the firmware isn't there. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 1ddf3a9..3cdd319 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -537,26 +537,6 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) return true; } -void i915_firmware_load_error_print(const char *fw_path, int err) -{ - DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err); - - /* - * If the reason is not known assume -ENOENT since that's the most - * usual failure mode. - */ - if (!err) - err = -ENOENT; - - if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT)) - return; - - DRM_ERROR( - "The driver is built-in, so to load the firmware you need to\n" - "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n" - "in your initrd/initramfs image.\n"); -} - static void intel_suspend_encoders(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9f01c72..6241b49 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2654,7 +2654,6 @@ extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv); extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv); extern void i915_update_gfx_val(struct drm_i915_private *dev_priv); int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on); -void i915_firmware_load_error_print(const char *fw_path, int err); /* intel_hotplug.c */ void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask); diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 00e9fbf..9d4b37b 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -237,10 +237,8 @@ static void finish_csr_load(const struct firmware *fw, void *context) uint32_t *dmc_payload; bool fw_loaded = false; - if (!fw) { - i915_firmware_load_error_print(csr->fw_path, 0); + if (!fw) goto out; - } if ((stepping == -ENODATA) || (substepping == -ENODATA)) { DRM_ERROR("Unknown stepping info, firmware loading failed\n"); @@ -340,6 +338,8 @@ static void finish_csr_load(const struct firmware *fw, void *context) out: if (fw_loaded || IS_BROXTON(dev)) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); + else + DRM_ERROR("Failed to load DMC firmware, disabling rpm\n"); release_firmware(fw); } @@ -380,8 +380,6 @@ void intel_csr_ucode_init(struct drm_device *dev) &dev_priv->dev->pdev->dev, GFP_KERNEL, dev_priv, finish_csr_load); - if (ret) - i915_firmware_load_error_print(csr->fw_path, ret); } /**