Re: [Intel-gfx] [PATCH] drm/i915/dg2: make GuC FW a requirement for Gen12 and beyond devices

[Robert Beckett <bob.beckett@xxxxxxxxxxxxx>]

On 07/12/2021 23:15, John Harrison wrote:
> On 12/7/2021 09:53, Adrian Larumbe wrote:
> > Beginning with DG2, all successive devices will require GuC FW to be
> > present and loaded at probe() time. This change alters error handling in
> > the FW init and load functions so that the driver's probe() function will
> > fail if GuC could not be loaded.
> We still need to load the i915 driver in fall back mode (display but no 
> engines) if the GuC is missing. Otherwise you may have just bricked the 
> user's device.

good point, well made.
though this still seems like an issue for gen12+ (excluding rkl and adl).

maybe a redesign of toplevel driver probe, with i915_driver_early_probe 
before i915_driver_create could work. If the GuC fw is not found, it 
could then register a new kms only version of i915_drm_driver.

or something like like that ...

> Also, we do want to be able to disable the GuC via the enable_guc module 
> parameter.
>
> John.
>
>
> > Signed-off-by: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 20 ++++++++++++++++----
> >   drivers/gpu/drm/i915/gt/uc/intel_uc.h |  4 ++--
> >   drivers/gpu/drm/i915/i915_gem.c       |  7 ++++++-
> >   3 files changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > index 7660eba893fa..8b0778b6d9ab 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > @@ -277,14 +277,19 @@ static void guc_disable_communication(struct 
> > intel_guc *guc)
> >       drm_dbg(&i915->drm, "GuC communication disabled\n");
> >   }
> > -static void __uc_fetch_firmwares(struct intel_uc *uc)
> > +static int __uc_fetch_firmwares(struct intel_uc *uc)
> >   {
> > +    struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
> >       int err;
> >       GEM_BUG_ON(!intel_uc_wants_guc(uc));
> >       err = intel_uc_fw_fetch(&uc->guc.fw);
> >       if (err) {
> > +        /* GuC is mandatory on Gen12 and beyond */
> > +        if (GRAPHICS_VER(i915) >= 12)
> > +            return err;
> > +
> >           /* Make sure we transition out of transient "SELECTED" state */
> >           if (intel_uc_wants_huc(uc)) {
> >               drm_dbg(&uc_to_gt(uc)->i915->drm,
> > @@ -293,11 +298,13 @@ static void __uc_fetch_firmwares(struct intel_uc 
> > *uc)
> >                             INTEL_UC_FIRMWARE_ERROR);
> >           }
> > -        return;
> > +        return 0;
> >       }
> >       if (intel_uc_wants_huc(uc))
> >           intel_uc_fw_fetch(&uc->huc.fw);
> > +
> > +    return 0;
> >   }
> >   static void __uc_cleanup_firmwares(struct intel_uc *uc)
> > @@ -308,14 +315,19 @@ static void __uc_cleanup_firmwares(struct 
> > intel_uc *uc)
> >   static int __uc_init(struct intel_uc *uc)
> >   {
> > +    struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
> >       struct intel_guc *guc = &uc->guc;
> >       struct intel_huc *huc = &uc->huc;
> >       int ret;
> >       GEM_BUG_ON(!intel_uc_wants_guc(uc));
> > -    if (!intel_uc_uses_guc(uc))
> > -        return 0;
> > +    if (!intel_uc_uses_guc(uc)) {
> > +        if (GRAPHICS_VER(i915) >= 12)
> > +            return -EINVAL;
> > +        else
> > +            return 0;
> > +    }
> >       if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
> >           return -ENOMEM;
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > index 866b462821c0..3bcd781447bc 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > @@ -17,7 +17,7 @@ struct intel_uc;
> >   struct intel_uc_ops {
> >       int (*sanitize)(struct intel_uc *uc);
> > -    void (*init_fw)(struct intel_uc *uc);
> > +    int (*init_fw)(struct intel_uc *uc);
> >       void (*fini_fw)(struct intel_uc *uc);
> >       int (*init)(struct intel_uc *uc);
> >       void (*fini)(struct intel_uc *uc);
> > @@ -104,7 +104,7 @@ static inline _TYPE intel_uc_##_NAME(struct 
> > intel_uc *uc) \
> >       return _RET; \
> >   }
> >   intel_uc_ops_function(sanitize, sanitize, int, 0);
> > -intel_uc_ops_function(fetch_firmwares, init_fw, void, );
> > +intel_uc_ops_function(fetch_firmwares, init_fw, int, 0);
> >   intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
> >   intel_uc_ops_function(init, init, int, 0);
> >   intel_uc_ops_function(fini, fini, void, );
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 527228d4da7e..7f8204af6826 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1049,7 +1049,12 @@ int i915_gem_init(struct drm_i915_private 
> > *dev_priv)
> >       if (ret)
> >           return ret;
> > -    intel_uc_fetch_firmwares(&dev_priv->gt.uc);
> > +    ret = intel_uc_fetch_firmwares(&dev_priv->gt.uc);
> > +    if (ret) {
> > +        i915_probe_error(dev_priv, "Failed to fetch firmware\n");
> > +        return ret;
> > +    }
> > +
> >       intel_wopcm_init(&dev_priv->wopcm);
> >       ret = i915_init_ggtt(dev_priv);