| Message ID | 20170224154004.17821-10-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 24, 2017 at 04:40:03PM +0100, Arkadiusz Hiler wrote: > intel_{h,g}uc_init_fw selects correct firmware and then triggers it's > preparation (fetch + initial parsing). > > This change separates out select steps, so those can be called by > the sanitize_options(). > > Then, during the init_fw() we prepare the firmware if the firmware was > selected. > > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- It looks strange that in one series you're changing function names or their logic few times ... patch ordering really matters ;) Please consider reorder/squash. > drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++---------- > drivers/gpu/drm/i915/intel_huc.c | 14 ++------------ > drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++------ > drivers/gpu/drm/i915/intel_uc.h | 4 ++-- > 4 files changed, 20 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 64f50bd..8ccd832 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -467,15 +467,10 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > } > > /** > - * intel_guc_init_fw() - select and prepare firmware for loading > + * intel_guc_select_fw() - selects GuC firmware for loading > * @guc: intel_guc struct > - * > - * Called early during driver load, but after GEM is initialised. > - * > - * The firmware will be transferred to the GuC's memory later, > - * when intel_guc_init_hw() is called. > */ > -void intel_guc_init_fw(struct intel_guc *guc) > +void intel_guc_select_fw(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > @@ -498,11 +493,8 @@ void intel_guc_init_fw(struct intel_guc *guc) > guc->fw.minor_ver_wanted = KBL_FW_MINOR; > } else { > DRM_ERROR("No GuC firmware known for platform with GuC!\n"); > - i915.enable_guc_loading = 0; > return; > } > - > - intel_uc_prepare_fw(dev_priv, &guc->fw); > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index 757f618..d073a68 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -141,18 +141,10 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv) > } > > /** > - * intel_huc_init_fw() - select and prepare firmware for loading > + * intel_huc_select_fw() - selects HuC firmware for loading > * @huc: intel_huc struct > - * > - * Called early during driver load, but after GEM is initialised. The loading > - * will continue only when driver explicitly specify firmware name and version. > - * All other cases are considered as INTEL_UC_FIRMWARE_NONE either because HW > - * is not capable or driver yet support it. And there will be no error message > - * for INTEL_UC_FIRMWARE_NONE cases. > - * > - * The DMA-copying to HW is done later when intel_huc_init_hw() is called. > */ > -void intel_huc_init_fw(struct intel_huc *huc) > +void intel_huc_select_fw(struct intel_huc *huc) > { > struct drm_i915_private *dev_priv = huc_to_i915(huc); > > @@ -177,8 +169,6 @@ void intel_huc_init_fw(struct intel_huc *huc) > DRM_ERROR("No HuC firmware known for platform with HuC!\n"); > return; > } > - > - intel_uc_prepare_fw(dev_priv, &huc->fw); > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index ac9ad59..89681b37 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > if (!i915.enable_guc_loading) > i915.enable_guc_submission = 0; > } Code below is only applicable to "else" from the above > + > + if (i915.enable_guc_loading) { > + if (HAS_HUC_UCODE(dev_priv)) > + intel_huc_select_fw(&dev_priv->huc); > + > + intel_guc_select_fw(&dev_priv->guc); > + > + if (!dev_priv->guc.fw.path) > + i915.enable_guc_loading = 0; Maybe simpler like this: i915.enable_guc_loading = intel_guc_select_fw(guc); and likely it can be done earlier too. > + } > } > > void intel_uc_init_early(struct drm_i915_private *dev_priv) > @@ -75,13 +85,11 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > > void intel_uc_init_fw(struct drm_i915_private *dev_priv) > { > - if (!i915.enable_guc_loading) > - return; > + if (dev_priv->huc.fw.path) Maybe we can move this common "if" to "intel_uc_prepare_fw()" ? -Michal > + intel_uc_prepare_fw(dev_priv, &dev_priv->huc.fw); > > - if (HAS_HUC_UCODE(dev_priv)) > - intel_huc_init_fw(&dev_priv->huc); > - > - intel_guc_init_fw(&dev_priv->guc); > + if (dev_priv->guc.fw.path) > + intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw); > } > > int intel_uc_init_hw(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index d19c95e..5f04ea1 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -194,7 +194,7 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, > struct intel_uc_fw *uc_fw); > > /* intel_guc_loader.c */ > -void intel_guc_init_fw(struct intel_guc *guc); > +void intel_guc_select_fw(struct intel_guc *guc); > int intel_guc_init_hw(struct drm_i915_private *dev_priv); > void intel_guc_fini(struct drm_i915_private *dev_priv); > const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status); > @@ -230,7 +230,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) > } > > /* intel_huc.c */ > -void intel_huc_init_fw(struct intel_huc *huc); > +void intel_huc_select_fw(struct intel_huc *huc); > void intel_huc_fini(struct drm_i915_private *dev_priv); > int intel_huc_init_hw(struct drm_i915_private *dev_priv); > void intel_guc_auth_huc(struct drm_i915_private *dev_priv); > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote: > intel_{h,g}uc_init_fw selects correct firmware and then triggers it's > preparation (fetch + initial parsing). > > This change separates out select steps, so those can be called by > the sanitize_options(). > > Then, during the init_fw() we prepare the firmware if the firmware was > selected. > > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> One comment below. > @@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > if (!i915.enable_guc_loading) > i915.enable_guc_submission = 0; > } > + > + if (i915.enable_guc_loading) { > + if (HAS_HUC_UCODE(dev_priv)) > + intel_huc_select_fw(&dev_priv->huc); > + > + intel_guc_select_fw(&dev_priv->guc); > + > + if (!dev_priv->guc.fw.path) > + i915.enable_guc_loading = 0; Maybe make this (as suggested by Michal): if (intel_guc_select_fw(&dev_priv->guc) < 0) i915.enable_guc_loading = 0; I think the select_fw is appropriate in this place once we introduce the option to override the FW path, it'll part of sanitization. Regards, Joonas
On Fri, Feb 24, 2017 at 07:29:57PM +0100, Michal Wajdeczko wrote: > On Fri, Feb 24, 2017 at 04:40:03PM +0100, Arkadiusz Hiler wrote: > > intel_{h,g}uc_init_fw selects correct firmware and then triggers it's > > preparation (fetch + initial parsing). > > > > This change separates out select steps, so those can be called by > > the sanitize_options(). > > > > Then, during the init_fw() we prepare the firmware if the firmware was > > selected. > > > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > It looks strange that in one series you're changing function names > or their logic few times ... patch ordering really matters ;) Is this really that bad? I do not see that much disadvantage to the current state. I am seeing the series as a whole. Titles of the commits are pretty informative (and you have them both in you email client when you are viewing the whole thread, and in the cover letter) - e.g. you can see that one of the later commits reworks firmware path handling, and can assume that the previous changes to that code were intermediary (changing argument it takes and removing one or two checks as they are now on the level above). As of the name changes - those follow a pattern. If I went with the original name until the final change it looks inconsistent, and quite probably someone would've commented on that. If I have had used the final names straight away that would be simply misleading (in case of intel_guc_init -> init_fw -> select_fw). > Please consider reorder/squash. I've tried to rebase with squashing and resplitting two times, and what I came with was pretty similar to the current series. But I might have hard-wired my brain to follow those exact steps, therefore the repetition... If you have any suggestion how to do that properly, keeping changes small, logical and ordered please share. I'd love to learn how to make my series less painful for future reviewers.
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 64f50bd..8ccd832 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -467,15 +467,10 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) } /** - * intel_guc_init_fw() - select and prepare firmware for loading + * intel_guc_select_fw() - selects GuC firmware for loading * @guc: intel_guc struct - * - * Called early during driver load, but after GEM is initialised. - * - * The firmware will be transferred to the GuC's memory later, - * when intel_guc_init_hw() is called. */ -void intel_guc_init_fw(struct intel_guc *guc) +void intel_guc_select_fw(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -498,11 +493,8 @@ void intel_guc_init_fw(struct intel_guc *guc) guc->fw.minor_ver_wanted = KBL_FW_MINOR; } else { DRM_ERROR("No GuC firmware known for platform with GuC!\n"); - i915.enable_guc_loading = 0; return; } - - intel_uc_prepare_fw(dev_priv, &guc->fw); } /** diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 757f618..d073a68 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -141,18 +141,10 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv) } /** - * intel_huc_init_fw() - select and prepare firmware for loading + * intel_huc_select_fw() - selects HuC firmware for loading * @huc: intel_huc struct - * - * Called early during driver load, but after GEM is initialised. The loading - * will continue only when driver explicitly specify firmware name and version. - * All other cases are considered as INTEL_UC_FIRMWARE_NONE either because HW - * is not capable or driver yet support it. And there will be no error message - * for INTEL_UC_FIRMWARE_NONE cases. - * - * The DMA-copying to HW is done later when intel_huc_init_hw() is called. */ -void intel_huc_init_fw(struct intel_huc *huc) +void intel_huc_select_fw(struct intel_huc *huc) { struct drm_i915_private *dev_priv = huc_to_i915(huc); @@ -177,8 +169,6 @@ void intel_huc_init_fw(struct intel_huc *huc) DRM_ERROR("No HuC firmware known for platform with HuC!\n"); return; } - - intel_uc_prepare_fw(dev_priv, &huc->fw); } /** diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index ac9ad59..89681b37 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -66,6 +66,16 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) if (!i915.enable_guc_loading) i915.enable_guc_submission = 0; } + + if (i915.enable_guc_loading) { + if (HAS_HUC_UCODE(dev_priv)) + intel_huc_select_fw(&dev_priv->huc); + + intel_guc_select_fw(&dev_priv->guc); + + if (!dev_priv->guc.fw.path) + i915.enable_guc_loading = 0; + } } void intel_uc_init_early(struct drm_i915_private *dev_priv) @@ -75,13 +85,11 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) void intel_uc_init_fw(struct drm_i915_private *dev_priv) { - if (!i915.enable_guc_loading) - return; + if (dev_priv->huc.fw.path) + intel_uc_prepare_fw(dev_priv, &dev_priv->huc.fw); - if (HAS_HUC_UCODE(dev_priv)) - intel_huc_init_fw(&dev_priv->huc); - - intel_guc_init_fw(&dev_priv->guc); + if (dev_priv->guc.fw.path) + intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw); } int intel_uc_init_hw(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index d19c95e..5f04ea1 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -194,7 +194,7 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, struct intel_uc_fw *uc_fw); /* intel_guc_loader.c */ -void intel_guc_init_fw(struct intel_guc *guc); +void intel_guc_select_fw(struct intel_guc *guc); int intel_guc_init_hw(struct drm_i915_private *dev_priv); void intel_guc_fini(struct drm_i915_private *dev_priv); const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status); @@ -230,7 +230,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) } /* intel_huc.c */ -void intel_huc_init_fw(struct intel_huc *huc); +void intel_huc_select_fw(struct intel_huc *huc); void intel_huc_fini(struct drm_i915_private *dev_priv); int intel_huc_init_hw(struct drm_i915_private *dev_priv); void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
intel_{h,g}uc_init_fw selects correct firmware and then triggers it's preparation (fetch + initial parsing). This change separates out select steps, so those can be called by the sanitize_options(). Then, during the init_fw() we prepare the firmware if the firmware was selected. Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++---------- drivers/gpu/drm/i915/intel_huc.c | 14 ++------------ drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++------ drivers/gpu/drm/i915/intel_uc.h | 4 ++-- 4 files changed, 20 insertions(+), 30 deletions(-)