| Message ID | 20181117004234.23437-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: allow to load DMC firmware on next gen | expand |
Quoting Lucas De Marchi (2018-11-17 00:42:34) > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > we have released no firmware yet for a platform, stop scaring the > consumer and merely note its expected absence. > > By simply removing the warning and early return we hit the condition > with the appropriate message. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index b4476d891fa3..a516697bf57d 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > csr->fw_path = BXT_CSR_PATH; > csr->required_version = BXT_CSR_VERSION_REQUIRED; > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > - } else { > - MISSING_CASE(INTEL_REVID(dev_priv)); Wider question would be if MISSING_CASE should be CI only. It's meant to be a warning for us to fix before alpha_support=0, and so should never reach users or any production system, so the question is moot. As we have a warning message then the MISSING_CASE is indeed superfluous here, and the firmware has a different release schedule to alpha_support so doesn't merit the tight coupling implied by MISSING_CASE (imo). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Quoting Chris Wilson (2018-11-19 00:01:57) > Quoting Lucas De Marchi (2018-11-17 00:42:34) > > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > > we have released no firmware yet for a platform, stop scaring the > > consumer and merely note its expected absence. > > > > By simply removing the warning and early return we hit the condition > > with the appropriate message. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_csr.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > index b4476d891fa3..a516697bf57d 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > > csr->fw_path = BXT_CSR_PATH; > > csr->required_version = BXT_CSR_VERSION_REQUIRED; > > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > > - } else { > > - MISSING_CASE(INTEL_REVID(dev_priv)); > > Wider question would be if MISSING_CASE should be CI only. It's meant to > be a warning for us to fix before alpha_support=0, and so should never > reach users or any production system, so the question is moot. Especially with the changes into CI log level reporting, we'd have to actually notice the MISSING_CASE triggers from the inflow of other IGT warn levels. MISSING_CASE triggering BUG if alpha_support flag is lifted might be enough of a hammer to drive the point home if we've missed some? Regards, Joonas > > As we have a warning message then the MISSING_CASE is indeed > superfluous here, and the firmware has a different release schedule to > alpha_support so doesn't merit the tight coupling implied by > MISSING_CASE (imo). > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris
On Mon, Nov 19, 2018 at 12:43:56PM +0200, Joonas Lahtinen wrote: > Quoting Chris Wilson (2018-11-19 00:01:57) > > Quoting Lucas De Marchi (2018-11-17 00:42:34) > > > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > > > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > > > we have released no firmware yet for a platform, stop scaring the > > > consumer and merely note its expected absence. > > > > > > By simply removing the warning and early return we hit the condition > > > with the appropriate message. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_csr.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > > index b4476d891fa3..a516697bf57d 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > > > csr->fw_path = BXT_CSR_PATH; > > > csr->required_version = BXT_CSR_VERSION_REQUIRED; > > > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > > > - } else { > > > - MISSING_CASE(INTEL_REVID(dev_priv)); > > > > Wider question would be if MISSING_CASE should be CI only. It's meant to > > be a warning for us to fix before alpha_support=0, and so should never > > reach users or any production system, so the question is moot. > > Especially with the changes into CI log level reporting, we'd have to > actually notice the MISSING_CASE triggers from the inflow of other IGT > warn levels. > > MISSING_CASE triggering BUG if alpha_support flag is lifted might be > enough of a hammer to drive the point home if we've missed some? I believe that would be too noise that we would never have a reliable CI. maybe even a chicken and egg for lifting alpha_support... But for this patch: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Regards, Joonas > > > > > As we have a warning message then the MISSING_CASE is indeed > > superfluous here, and the firmware has a different release schedule to > > alpha_support so doesn't merit the tight coupling implied by > > MISSING_CASE (imo). > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > -Chris
On Fri, 16 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > we have released no firmware yet for a platform, stop scaring the > consumer and merely note its expected absence. > > By simply removing the warning and early return we hit the condition > with the appropriate message. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> *sigh* This patch would not have been needed at all with patch 1/2. The point was that we shouldn't proceed without the max fw size set. Even with the module parameter. Because that would fail at the firmware loading stage later. If the warn was such a big deal, it should've been changed to a debug message with an early return, not removed. BR, Jani. > --- > drivers/gpu/drm/i915/intel_csr.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index b4476d891fa3..a516697bf57d 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > csr->fw_path = BXT_CSR_PATH; > csr->required_version = BXT_CSR_VERSION_REQUIRED; > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > - } else { > - MISSING_CASE(INTEL_REVID(dev_priv)); > - return; > } > > if (i915_modparams.dmc_firmware_path) {
On Wed, Nov 21, 2018 at 11:29:02AM +0200, Jani Nikula wrote: > On Fri, 16 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > > we have released no firmware yet for a platform, stop scaring the > > consumer and merely note its expected absence. > > > > By simply removing the warning and early return we hit the condition > > with the appropriate message. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > *sigh* ops, I just noticed that I forgot to reply here yesterday after merging these to dinq. :/ > > This patch would not have been needed at all with patch 1/2. > > The point was that we shouldn't proceed without the max fw size > set. Even with the module parameter. Because that would fail at the > firmware loading stage later. > > If the warn was such a big deal, it should've been changed to a debug > message with an early return, not removed. hmmm... that's a good point... > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/intel_csr.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > index b4476d891fa3..a516697bf57d 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > > csr->fw_path = BXT_CSR_PATH; > > csr->required_version = BXT_CSR_VERSION_REQUIRED; > > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > > - } else { > > - MISSING_CASE(INTEL_REVID(dev_priv)); > > - return; > > } > > > > if (i915_modparams.dmc_firmware_path) { > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 21, 2018 at 11:29:02AM +0200, Jani Nikula wrote: > On Fri, 16 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown > > firmware warnings") for huc and guc: downgrade CSR firmware warnings. If > > we have released no firmware yet for a platform, stop scaring the > > consumer and merely note its expected absence. > > > > By simply removing the warning and early return we hit the condition > > with the appropriate message. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > *sigh* > > This patch would not have been needed at all with patch 1/2. > > The point was that we shouldn't proceed without the max fw size > set. Even with the module parameter. Because that would fail at the > firmware loading stage later. > > If the warn was such a big deal, it should've been changed to a debug > message with an early return, not removed. This was only done to make the code similar to the huc/guc. As I see this would only be a problem in case we did pass a firmware via param and not being gen >= 12. Otherwise the warning (for !alpha) and debug message is unified in a single place, down a few lines in the same function. Lucas De Marchi > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/intel_csr.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > > index b4476d891fa3..a516697bf57d 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > > csr->fw_path = BXT_CSR_PATH; > > csr->required_version = BXT_CSR_VERSION_REQUIRED; > > csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; > > - } else { > > - MISSING_CASE(INTEL_REVID(dev_priv)); > > - return; > > } > > > > if (i915_modparams.dmc_firmware_path) { > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index b4476d891fa3..a516697bf57d 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -496,9 +496,6 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) csr->fw_path = BXT_CSR_PATH; csr->required_version = BXT_CSR_VERSION_REQUIRED; csr->max_fw_size = BXT_CSR_MAX_FW_SIZE; - } else { - MISSING_CASE(INTEL_REVID(dev_priv)); - return; } if (i915_modparams.dmc_firmware_path) {
Like it was done in commit 9e180d9991dc ("drm/i915: Downgrade unknown firmware warnings") for huc and guc: downgrade CSR firmware warnings. If we have released no firmware yet for a platform, stop scaring the consumer and merely note its expected absence. By simply removing the warning and early return we hit the condition with the appropriate message. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 3 --- 1 file changed, 3 deletions(-)