| Message ID | 20180906082126.15539-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/csr: keep firmware name and required version together | expand |
Quoting Jani Nikula (2018-09-06 09:21:24) > Having two separate if ladders gets increasingly hard to maintain. Put > them together. Does it even have to be an if-ladder? Something like struct platform_requirements { unsigned long platform_mask; u32 required_version; const char *path; } []; How does that work by patch 3? -Chris
On Thu, 06 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jani Nikula (2018-09-06 09:21:24) >> Having two separate if ladders gets increasingly hard to maintain. Put >> them together. > > Does it even have to be an if-ladder? Something like > struct platform_requirements { > unsigned long platform_mask; > u32 required_version; > const char *path; > } []; > > How does that work by patch 3? I expect this to evolve to the same kind of thing as the PCH detection and checks where you can't get away with a simple platform mask, and you have to resort to an if ladder. Or you need separate match functions, which get unwieldy without lambda functions. Also, I think we may need to have several firmware blobs per platform that get requested in sequence, to allow for more graceful updates. In any case, I think grouping this stuff together is a good first step. BR, Jani.
Quoting Jani Nikula (2018-09-06 09:21:24) > Having two separate if ladders gets increasingly hard to maintain. Put > them together. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_csr.c | 54 ++++++++++++++++------------------------ > 2 files changed, 23 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 767615ecdea5..368066010f94 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -464,6 +464,7 @@ struct drm_i915_display_funcs { > struct intel_csr { > struct work_struct work; > const char *fw_path; > + uint32_t required_version; > uint32_t *dmc_payload; > uint32_t dmc_fw_size; > uint32_t version; > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 14cf4c367e36..9a60bb9cc443 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -284,7 +284,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, > uint32_t max_fw_size = 0; > uint32_t i; > uint32_t *dmc_payload; > - uint32_t required_version; > > if (!fw) > return NULL; > @@ -299,36 +298,19 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, > return NULL; > } > > - csr->version = css_header->version; > - > - if (csr->fw_path == i915_modparams.dmc_firmware_path) { > - /* Bypass version check for firmware override. */ > - required_version = csr->version; > - } else if (IS_CANNONLAKE(dev_priv)) { > - required_version = CNL_CSR_VERSION_REQUIRED; > - } else if (IS_GEMINILAKE(dev_priv)) { > - required_version = GLK_CSR_VERSION_REQUIRED; > - } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) { > - required_version = KBL_CSR_VERSION_REQUIRED; > - } else if (IS_SKYLAKE(dev_priv)) { > - required_version = SKL_CSR_VERSION_REQUIRED; > - } else if (IS_BROXTON(dev_priv)) { > - required_version = BXT_CSR_VERSION_REQUIRED; > - } else { > - MISSING_CASE(INTEL_REVID(dev_priv)); > - required_version = 0; > - } > - > - if (csr->version != required_version) { > + if (csr->required_version && > + css_header->version != csr->required_version) { > DRM_INFO("Refusing to load DMC firmware v%u.%u," > " please use v%u.%u\n", > - CSR_VERSION_MAJOR(csr->version), > - CSR_VERSION_MINOR(csr->version), > - CSR_VERSION_MAJOR(required_version), > - CSR_VERSION_MINOR(required_version)); > + CSR_VERSION_MAJOR(css_header->version), > + CSR_VERSION_MINOR(css_header->version), > + CSR_VERSION_MAJOR(csr->required_version), > + CSR_VERSION_MINOR(csr->required_version)); > return NULL; > } > > + csr->version = css_header->version; Ok, so far. > + > readcount += sizeof(struct intel_css_header); > > /* Extract Package Header information*/ > @@ -469,18 +451,26 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) > if (!HAS_CSR(dev_priv)) > return; > > - if (i915_modparams.dmc_firmware_path) > + if (i915_modparams.dmc_firmware_path) { > csr->fw_path = i915_modparams.dmc_firmware_path; > - else if (IS_CANNONLAKE(dev_priv)) > + /* Bypass version check for firmware override. */ > + csr->required_version = 0; Ok. > + } else if (IS_CANNONLAKE(dev_priv)) { > csr->fw_path = I915_CSR_CNL; > - else if (IS_GEMINILAKE(dev_priv)) > + csr->required_version = CNL_CSR_VERSION_REQUIRED; Ok. > + } else if (IS_GEMINILAKE(dev_priv)) { > csr->fw_path = I915_CSR_GLK; > - else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) > + csr->required_version = GLK_CSR_VERSION_REQUIRED; Ok. > + } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) { > csr->fw_path = I915_CSR_KBL; > - else if (IS_SKYLAKE(dev_priv)) > + csr->required_version = KBL_CSR_VERSION_REQUIRED; Ok. > + } else if (IS_SKYLAKE(dev_priv)) { > csr->fw_path = I915_CSR_SKL; > - else if (IS_BROXTON(dev_priv)) > + csr->required_version = SKL_CSR_VERSION_REQUIRED; Ok. > + } else if (IS_BROXTON(dev_priv)) { > csr->fw_path = I915_CSR_BXT; > + csr->required_version = BXT_CSR_VERSION_REQUIRED; Ok. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 767615ecdea5..368066010f94 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -464,6 +464,7 @@ struct drm_i915_display_funcs { struct intel_csr { struct work_struct work; const char *fw_path; + uint32_t required_version; uint32_t *dmc_payload; uint32_t dmc_fw_size; uint32_t version; diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 14cf4c367e36..9a60bb9cc443 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -284,7 +284,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, uint32_t max_fw_size = 0; uint32_t i; uint32_t *dmc_payload; - uint32_t required_version; if (!fw) return NULL; @@ -299,36 +298,19 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv, return NULL; } - csr->version = css_header->version; - - if (csr->fw_path == i915_modparams.dmc_firmware_path) { - /* Bypass version check for firmware override. */ - required_version = csr->version; - } else if (IS_CANNONLAKE(dev_priv)) { - required_version = CNL_CSR_VERSION_REQUIRED; - } else if (IS_GEMINILAKE(dev_priv)) { - required_version = GLK_CSR_VERSION_REQUIRED; - } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) { - required_version = KBL_CSR_VERSION_REQUIRED; - } else if (IS_SKYLAKE(dev_priv)) { - required_version = SKL_CSR_VERSION_REQUIRED; - } else if (IS_BROXTON(dev_priv)) { - required_version = BXT_CSR_VERSION_REQUIRED; - } else { - MISSING_CASE(INTEL_REVID(dev_priv)); - required_version = 0; - } - - if (csr->version != required_version) { + if (csr->required_version && + css_header->version != csr->required_version) { DRM_INFO("Refusing to load DMC firmware v%u.%u," " please use v%u.%u\n", - CSR_VERSION_MAJOR(csr->version), - CSR_VERSION_MINOR(csr->version), - CSR_VERSION_MAJOR(required_version), - CSR_VERSION_MINOR(required_version)); + CSR_VERSION_MAJOR(css_header->version), + CSR_VERSION_MINOR(css_header->version), + CSR_VERSION_MAJOR(csr->required_version), + CSR_VERSION_MINOR(csr->required_version)); return NULL; } + csr->version = css_header->version; + readcount += sizeof(struct intel_css_header); /* Extract Package Header information*/ @@ -469,18 +451,26 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv) if (!HAS_CSR(dev_priv)) return; - if (i915_modparams.dmc_firmware_path) + if (i915_modparams.dmc_firmware_path) { csr->fw_path = i915_modparams.dmc_firmware_path; - else if (IS_CANNONLAKE(dev_priv)) + /* Bypass version check for firmware override. */ + csr->required_version = 0; + } else if (IS_CANNONLAKE(dev_priv)) { csr->fw_path = I915_CSR_CNL; - else if (IS_GEMINILAKE(dev_priv)) + csr->required_version = CNL_CSR_VERSION_REQUIRED; + } else if (IS_GEMINILAKE(dev_priv)) { csr->fw_path = I915_CSR_GLK; - else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) + csr->required_version = GLK_CSR_VERSION_REQUIRED; + } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) { csr->fw_path = I915_CSR_KBL; - else if (IS_SKYLAKE(dev_priv)) + csr->required_version = KBL_CSR_VERSION_REQUIRED; + } else if (IS_SKYLAKE(dev_priv)) { csr->fw_path = I915_CSR_SKL; - else if (IS_BROXTON(dev_priv)) + csr->required_version = SKL_CSR_VERSION_REQUIRED; + } else if (IS_BROXTON(dev_priv)) { csr->fw_path = I915_CSR_BXT; + csr->required_version = BXT_CSR_VERSION_REQUIRED; + } /* * Obtain a runtime pm reference, until CSR is loaded,
Having two separate if ladders gets increasingly hard to maintain. Put them together. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_csr.c | 54 ++++++++++++++++------------------------ 2 files changed, 23 insertions(+), 32 deletions(-)