On Fri, Mar 10, 2017 at 01:11:06PM +0100, Michal Wajdeczko wrote:
> On Fri, Mar 10, 2017 at 12:46:06PM +0100, Arkadiusz Hiler wrote:
> > Current version of intel_guc_init_hw() does a lot:
> > - cares about submission
> > - loads huc
> > - implement WA
> >
> > This change offloads some of the logic to intel_uc_init_hw(), which now
> > cares about the above.
> >
> > v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> > v3: rename once again
> > v4: remove spurious comments and add some style (J. Lahtinen)
> > v5: flow changes, got rid of dead checks (M. Wajdeczko)
> > v6: rebase
> >
> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> > ---
>
> snip
>
> > @@ -397,42 +379,22 @@ int intel_guc_init_hw(struct intel_guc *guc)
> > {
> > struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > const char *fw_path = guc->fw.path;
> > - int retries, ret, err;
> > + int ret;
> >
> > DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> > fw_path,
> > intel_uc_fw_status_repr(guc->fw.fetch_status),
> > intel_uc_fw_status_repr(guc->fw.load_status));
> >
> > - /* Loading forbidden, or no firmware to load? */
> > - if (!i915.enable_guc_loading) {
> > - err = 0;
> > - goto fail;
> > - } else if (fw_path == NULL) {
> > - /* Device is known to have no uCode (e.g. no GuC) */
> > - err = -ENXIO;
> > - goto fail;
> > + if (!fw_path) {
> > + 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");
> > - err = -ENODEV;
> > - goto fail;
> > + return -ENODEV;
> > }
> >
> > - /* Fetch failed, or already fetched but failed to load? */
> > - if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> > - err = -EIO;
> > - goto fail;
> > - } else if (guc->fw.load_status == INTEL_UC_FIRMWARE_FAIL) {
> > - err = -ENOEXEC;
> > - goto fail;
> > - }
> > -
> > - guc_interrupts_release(dev_priv);
> > - gen9_reset_guc_interrupts(dev_priv);
> > -
> > - /* We need to notify the guc whenever we change the GGTT */
> > - i915_ggtt_enable_guc(dev_priv);
> > + if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > + return -EIO;
>
> Above fw_path checks seem to be redundant as we also look for fetch status here.
Got rid of it in the path simplifying patch. Thanks.
> > @@ -63,6 +84,98 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> > intel_guc_init_fw(&dev_priv->guc);
> > }
> >
> > +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > +{
> > + int ret, attempts;
> > + const int guc_wa_hash_check_not_set_attempts = 3;
>
> In all other places, we put const vars definitions as first statements.
> And maybe this const should be accompanied by the /* Wa */ comment
> that is shown below. Also changing prefix to "gen9" maybe helpful
Went with using it directly, as Joonas suggested. The current if-else
statement makes meaning obvious enough without having the need to name
it.
--
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
