| Message ID | 20170217130557.6710-9-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 17, 2017 at 02:05:57PM +0100, Arkadiusz Hiler wrote: Typo in subject s/firwmare/firmware > Currently fw->path values can represent one of three possible states: > > 1) NULL - device without the uC > 2) '\0' - device with the uC but have no firmware > 3) else - device with the uC and we have firmware > > Second case is used only to WARN at a later stage. > > We can WARN right away and merge cases 1 and 2. > > Code can be even further simplified and common (HuC/GuC logic) happening > right before the fetch can be offloaded to the common function. > > v2: fewer temporary variables, more straightforward flow (M. Wajdeczko) > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++---------------------- > drivers/gpu/drm/i915/intel_huc.c | 20 +++++------------ > drivers/gpu/drm/i915/intel_uc.c | 5 +++-- > 3 files changed, 22 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 549a254..aade185 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > intel_uc_fw_status_repr(guc_fw->load_status)); > > if (fw_path == NULL) { > - /* Device is known to have no uCode (e.g. no GuC) */ > + /* We do not have uCode for the device */ > return -ENXIO; > - } else if (*fw_path == '\0') { > - /* Device has a GuC but we don't know what f/w to load? */ > - WARN(1, "No GuC firmware known for this platform!\n"); > - return -ENODEV; > } > > /* Fetch failed, or already fetched but failed to load? */ > @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > return 0; > } > > - > /** > * intel_guc_fetch_fw() - determine and fetch firmware > * @guc: intel_guc struct > @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > void intel_guc_fetch_fw(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > - const char *fw_path; > > - if (!HAS_GUC_UCODE(dev_priv)) { > - fw_path = NULL; > - } else if (IS_SKYLAKE(dev_priv)) { > - fw_path = I915_SKL_GUC_UCODE; > + guc->fw.path = NULL; > + guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; > + guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > + guc->fw.fw = INTEL_UC_FW_TYPE_GUC; Maybe for above code we can add new function: void intel_uc_fw_init_early(struct intel_uc_fw *fw, enum intel_uc_fw_type type); and use it for both guc and huc: intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC); intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC); > + > + if (IS_SKYLAKE(dev_priv)) { > + guc->fw.path = I915_SKL_GUC_UCODE; > guc->fw.major_ver_wanted = SKL_FW_MAJOR; > guc->fw.minor_ver_wanted = SKL_FW_MINOR; > } else if (IS_BROXTON(dev_priv)) { > - fw_path = I915_BXT_GUC_UCODE; > + guc->fw.path = I915_BXT_GUC_UCODE; > guc->fw.major_ver_wanted = BXT_FW_MAJOR; > guc->fw.minor_ver_wanted = BXT_FW_MINOR; > } else if (IS_KABYLAKE(dev_priv)) { > - fw_path = I915_KBL_GUC_UCODE; > + guc->fw.path = I915_KBL_GUC_UCODE; > guc->fw.major_ver_wanted = KBL_FW_MAJOR; > guc->fw.minor_ver_wanted = KBL_FW_MINOR; > } else { > - fw_path = ""; /* unknown device */ > + WARN(1, "No GuC firmware known for platform with GuC!\n"); Maybe simpler DRM_ERROR will be sufficient? We don't need callstack. > + i915.enable_guc_loading = 0; What about making this firmware path guess work part of the early init or sanitize options function? Note that actual fetch is already done by in different function, so mostly we just need to pick nice name for the new function. Maybe int intel_guc_init_fw() ? Note that changing i915.enable_guc param here has implication on other actions (like Huc loading) and thus forcing redundant checks elsewhere > + return; > } > > - guc->fw.path = fw_path; > - guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; > - guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > - > - if (fw_path == NULL) > - return; > - if (*fw_path == '\0') > - return; > - > - guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING; > - DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path); > guc->fw.fetch(dev_priv, &guc->fw); > - /* status must now be FAIL or SUCCESS */ > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index 7527988..c94e22f 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -155,37 +155,29 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv) > void intel_huc_fetch_fw(struct intel_huc *huc) > { > struct drm_i915_private *dev_priv = huc_to_i915(huc); > - const char *fw_path = NULL; > > huc->fw.path = NULL; > huc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; > huc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > huc->fw.fw = INTEL_UC_FW_TYPE_HUC; > > - if (!HAS_HUC_UCODE(dev_priv)) > - return; > - > if (IS_SKYLAKE(dev_priv)) { > - fw_path = I915_SKL_HUC_UCODE; > + huc->fw.path = I915_SKL_HUC_UCODE; > huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR; > huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR; > } else if (IS_BROXTON(dev_priv)) { > - fw_path = I915_BXT_HUC_UCODE; > + huc->fw.path = I915_BXT_HUC_UCODE; > huc->fw.major_ver_wanted = BXT_HUC_FW_MAJOR; > huc->fw.minor_ver_wanted = BXT_HUC_FW_MINOR; > } else if (IS_KABYLAKE(dev_priv)) { > - fw_path = I915_KBL_HUC_UCODE; > + huc->fw.path = I915_KBL_HUC_UCODE; > huc->fw.major_ver_wanted = KBL_HUC_FW_MAJOR; > huc->fw.minor_ver_wanted = KBL_HUC_FW_MINOR; > + } else { > + WARN(1, "No HuC firmware known for platform with HuC!\n"); > + return; > } > > - huc->fw.path = fw_path; > - huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING; > - > - DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path); > - > - WARN(huc->fw.path == NULL, "HuC present but no fw path\n"); > - > huc->fw.fetch(dev_priv, &huc->fw); > } > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 2bb49b7..6f2d3f6 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv, > size_t size; > int err; > > - DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n", > - intel_uc_fw_status_repr(uc_fw->fetch_status)); > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING; > + > + DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path); If you are not planning to remove "intel_uc_fw_status_repr()" then please it. -Michal > > err = request_firmware(&fw, uc_fw->path, &pdev->dev); > if (err) > -- > 2.9.3 >
On Fri, Feb 17, 2017 at 03:29:17PM +0100, Michal Wajdeczko wrote: > On Fri, Feb 17, 2017 at 02:05:57PM +0100, Arkadiusz Hiler wrote: > > Typo in subject s/firwmare/firmware > > > > Currently fw->path values can represent one of three possible states: > > > > 1) NULL - device without the uC > > 2) '\0' - device with the uC but have no firmware > > 3) else - device with the uC and we have firmware > > > > Second case is used only to WARN at a later stage. > > > > We can WARN right away and merge cases 1 and 2. > > > > Code can be even further simplified and common (HuC/GuC logic) happening > > right before the fetch can be offloaded to the common function. > > > > v2: fewer temporary variables, more straightforward flow (M. Wajdeczko) > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++---------------------- > > drivers/gpu/drm/i915/intel_huc.c | 20 +++++------------ > > drivers/gpu/drm/i915/intel_uc.c | 5 +++-- > > 3 files changed, 22 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 549a254..aade185 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > > intel_uc_fw_status_repr(guc_fw->load_status)); > > > > if (fw_path == NULL) { > > - /* Device is known to have no uCode (e.g. no GuC) */ > > + /* We do not have uCode for the device */ > > return -ENXIO; > > - } else if (*fw_path == '\0') { > > - /* Device has a GuC but we don't know what f/w to load? */ > > - WARN(1, "No GuC firmware known for this platform!\n"); > > - return -ENODEV; > > } > > > > /* Fetch failed, or already fetched but failed to load? */ > > @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > > return 0; > > } > > > > - > > /** > > * intel_guc_fetch_fw() - determine and fetch firmware > > * @guc: intel_guc struct > > @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > > void intel_guc_fetch_fw(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - const char *fw_path; > > > > - if (!HAS_GUC_UCODE(dev_priv)) { > > - fw_path = NULL; > > - } else if (IS_SKYLAKE(dev_priv)) { > > - fw_path = I915_SKL_GUC_UCODE; > > + guc->fw.path = NULL; > > + guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; > > + guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > > + guc->fw.fw = INTEL_UC_FW_TYPE_GUC; > > Maybe for above code we can add new function: > > void intel_uc_fw_init_early(struct intel_uc_fw *fw, > enum intel_uc_fw_type type); > > and use it for both guc and huc: > > intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC); > intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC); That's something I want to get to, but this serries is already getting pretty swollen and this would mean yet another patch in the series, yet another round of review Let stick not only to digestible patches but digestible series as well. > > + > > + if (IS_SKYLAKE(dev_priv)) { > > + guc->fw.path = I915_SKL_GUC_UCODE; > > guc->fw.major_ver_wanted = SKL_FW_MAJOR; > > guc->fw.minor_ver_wanted = SKL_FW_MINOR; > > } else if (IS_BROXTON(dev_priv)) { > > - fw_path = I915_BXT_GUC_UCODE; > > + guc->fw.path = I915_BXT_GUC_UCODE; > > guc->fw.major_ver_wanted = BXT_FW_MAJOR; > > guc->fw.minor_ver_wanted = BXT_FW_MINOR; > > } else if (IS_KABYLAKE(dev_priv)) { > > - fw_path = I915_KBL_GUC_UCODE; > > + guc->fw.path = I915_KBL_GUC_UCODE; > > guc->fw.major_ver_wanted = KBL_FW_MAJOR; > > guc->fw.minor_ver_wanted = KBL_FW_MINOR; > > } else { > > - fw_path = ""; /* unknown device */ > > + WARN(1, "No GuC firmware known for platform with GuC!\n"); > > Maybe simpler DRM_ERROR will be sufficient? We don't need callstack. I just stuck to what was already used, but DRM_ERROR should be sufficient. > > + i915.enable_guc_loading = 0; > > What about making this firmware path guess work part of the early init or > sanitize options function? Note that actual fetch is already done by in > different function, so mostly we just need to pick nice name for the > new function. Maybe > > int intel_guc_init_fw() ? > > Note that changing i915.enable_guc param here has implication on other > actions (like Huc loading) and thus forcing redundant checks elsewhere Same point as with intel_uc_fw_init_early. On my TODO for vol. 2. > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index 2bb49b7..6f2d3f6 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv, > > size_t size; > > int err; > > > > - DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n", > > - intel_uc_fw_status_repr(uc_fw->fetch_status)); > > + uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING; > > + > > + DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path); > > If you are not planning to remove "intel_uc_fw_status_repr()" then please it. What?
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 549a254..aade185 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) intel_uc_fw_status_repr(guc_fw->load_status)); if (fw_path == NULL) { - /* Device is known to have no uCode (e.g. no GuC) */ + /* We do not have uCode for the device */ return -ENXIO; - } else if (*fw_path == '\0') { - /* Device has a GuC but we don't know what f/w to load? */ - WARN(1, "No GuC firmware known for this platform!\n"); - return -ENODEV; } /* Fetch failed, or already fetched but failed to load? */ @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) return 0; } - /** * intel_guc_fetch_fw() - determine and fetch firmware * @guc: intel_guc struct @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) void intel_guc_fetch_fw(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - const char *fw_path; - if (!HAS_GUC_UCODE(dev_priv)) { - fw_path = NULL; - } else if (IS_SKYLAKE(dev_priv)) { - fw_path = I915_SKL_GUC_UCODE; + guc->fw.path = NULL; + guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; + guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; + guc->fw.fw = INTEL_UC_FW_TYPE_GUC; + + if (IS_SKYLAKE(dev_priv)) { + guc->fw.path = I915_SKL_GUC_UCODE; guc->fw.major_ver_wanted = SKL_FW_MAJOR; guc->fw.minor_ver_wanted = SKL_FW_MINOR; } else if (IS_BROXTON(dev_priv)) { - fw_path = I915_BXT_GUC_UCODE; + guc->fw.path = I915_BXT_GUC_UCODE; guc->fw.major_ver_wanted = BXT_FW_MAJOR; guc->fw.minor_ver_wanted = BXT_FW_MINOR; } else if (IS_KABYLAKE(dev_priv)) { - fw_path = I915_KBL_GUC_UCODE; + guc->fw.path = I915_KBL_GUC_UCODE; guc->fw.major_ver_wanted = KBL_FW_MAJOR; guc->fw.minor_ver_wanted = KBL_FW_MINOR; } else { - fw_path = ""; /* unknown device */ + WARN(1, "No GuC firmware known for platform with GuC!\n"); + i915.enable_guc_loading = 0; + return; } - guc->fw.path = fw_path; - guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; - guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; - - if (fw_path == NULL) - return; - if (*fw_path == '\0') - return; - - guc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING; - DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path); guc->fw.fetch(dev_priv, &guc->fw); - /* status must now be FAIL or SUCCESS */ } /** diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 7527988..c94e22f 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -155,37 +155,29 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv) void intel_huc_fetch_fw(struct intel_huc *huc) { struct drm_i915_private *dev_priv = huc_to_i915(huc); - const char *fw_path = NULL; huc->fw.path = NULL; huc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE; huc->fw.load_status = INTEL_UC_FIRMWARE_NONE; huc->fw.fw = INTEL_UC_FW_TYPE_HUC; - if (!HAS_HUC_UCODE(dev_priv)) - return; - if (IS_SKYLAKE(dev_priv)) { - fw_path = I915_SKL_HUC_UCODE; + huc->fw.path = I915_SKL_HUC_UCODE; huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR; huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR; } else if (IS_BROXTON(dev_priv)) { - fw_path = I915_BXT_HUC_UCODE; + huc->fw.path = I915_BXT_HUC_UCODE; huc->fw.major_ver_wanted = BXT_HUC_FW_MAJOR; huc->fw.minor_ver_wanted = BXT_HUC_FW_MINOR; } else if (IS_KABYLAKE(dev_priv)) { - fw_path = I915_KBL_HUC_UCODE; + huc->fw.path = I915_KBL_HUC_UCODE; huc->fw.major_ver_wanted = KBL_HUC_FW_MAJOR; huc->fw.minor_ver_wanted = KBL_HUC_FW_MINOR; + } else { + WARN(1, "No HuC firmware known for platform with HuC!\n"); + return; } - huc->fw.path = fw_path; - huc->fw.fetch_status = INTEL_UC_FIRMWARE_PENDING; - - DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path); - - WARN(huc->fw.path == NULL, "HuC present but no fw path\n"); - huc->fw.fetch(dev_priv, &huc->fw); } diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 2bb49b7..6f2d3f6 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv, size_t size; int err; - DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n", - intel_uc_fw_status_repr(uc_fw->fetch_status)); + uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING; + + DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path); err = request_firmware(&fw, uc_fw->path, &pdev->dev); if (err)
Currently fw->path values can represent one of three possible states: 1) NULL - device without the uC 2) '\0' - device with the uC but have no firmware 3) else - device with the uC and we have firmware Second case is used only to WARN at a later stage. We can WARN right away and merge cases 1 and 2. Code can be even further simplified and common (HuC/GuC logic) happening right before the fetch can be offloaded to the common function. v2: fewer temporary variables, more straightforward flow (M. Wajdeczko) Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++---------------------- drivers/gpu/drm/i915/intel_huc.c | 20 +++++------------ drivers/gpu/drm/i915/intel_uc.c | 5 +++-- 3 files changed, 22 insertions(+), 42 deletions(-)