Re: [CI-RESEND 3/3] drm/i915/guc: revisit GuC loader message levels

[Dave Gordon <david.s.gordon@xxxxxxxxx>]

On 19/07/16 15:26, Tvrtko Ursulin wrote:
> On 19/07/16 13:20, Dave Gordon wrote:
> > Some downgraded from DRM_ERROR() to DRM_WARN() or DRM_NOTE(),
> > a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(),
> > and one eliminated completely.
> >
> > v2: different permutation of levels :)
> >
> > Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/intel_guc_loader.c | 34
> > ++++++++++++++++-----------------
> >   1 file changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> > b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 605c696..a2f4fa4 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private
> > *dev_priv)
> >
> >   static u32 get_core_family(struct drm_i915_private *dev_priv)
> >   {
> > -    switch (INTEL_INFO(dev_priv)->gen) {
> > +    u32 gen = INTEL_GEN(dev_priv);
> > +
> > +    switch (gen) {
> >       case 9:
> >           return GFXCORE_FAMILY_GEN9;
> >
> >       default:
> > -        DRM_ERROR("GUC: unsupported core family\n");
> > +        DRM_WARN("GEN%d does not support GuC operation\n", gen);
>
> This looks more like a WARN_ON condition to me, something developers
> need to notice extremely easily in development and will never happen in
> deployment.

OK; the check in the caller below should have prevented us reaching this 
code if the hardware ID isn't known to the driver. Changed to WARN(1, ...).

> >           return GFXCORE_FAMILY_UNKNOWN;
> >       }
> >   }
> > @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev)
> >           goto fail;
> >       } else if (*fw_path == '\0') {
> >           /* Device has a GuC but we don't know what f/w to load? */
> > -        DRM_INFO("No GuC firmware known for this platform\n");
> > +        DRM_WARN("No GuC firmware known for this platform\n");
>
> This looks the same to me (WARN_ON), it can only happen if someone
> messes up things in development.

Maybe. This is the outer check that protects the code above, and as such 
it's the first place we report unrecognised hardware. But we don't want 
to prevent the driver from working (via fallback to execlists) so 
developers can at least boot new hardware and not just get a blank 
screen. So a WARNING of some type is right, but does the stack trace 
from WARN() add any value? If not -- and I think not -- DRM_WARN() is 
what we want.

> >           err = -ENODEV;
> >           goto fail;
> >       }
> > @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev)
> >            * that the state and timing are fairly predictable
> >            */
> >           err = i915_reset_guc(dev_priv);
> > -        if (err) {
> > -            DRM_ERROR("GuC reset failed: %d\n", err);
> > +        if (err)
> >               goto fail;
> > -        }
> >
> >           err = guc_ucode_xfer(dev_priv);
> >           if (!err)
> > @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev)
> >       else if (err == 0)
> >           DRM_INFO("GuC firmware load skipped\n");
> >       else if (ret != -EIO)
> > -        DRM_INFO("GuC firmware load failed: %d\n", err);
> > +        DRM_NOTE("GuC firmware load failed: %d\n", err);
> >       else
> > -        DRM_ERROR("GuC firmware load failed: %d\n", err);
> > +        DRM_WARN("GuC firmware load failed: %d\n", err);
> >
> >       if (i915.enable_guc_submission) {
> >           if (fw_path == NULL)
> >               DRM_INFO("GuC submission without firmware not
> > supported\n");
> >           if (ret == 0)
> > -            DRM_INFO("Falling back from GuC submission to execlist
> > mode\n");
> > +            DRM_NOTE("Falling back from GuC submission to execlist
> > mode\n");
> >           else
> >               DRM_ERROR("GuC init failed: %d\n", ret);
> >       }
> > @@ -571,7 +571,7 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >
> >       /* Check the size of the blob before examining buffer contents */
> >       if (fw->size < sizeof(struct guc_css_header)) {
> > -        DRM_ERROR("Firmware header is missing\n");
> > +        DRM_NOTE("Firmware header is missing\n");
> >           goto fail;
> >       }
> >
> > @@ -583,7 +583,7 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >           css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
> >
> >       if (guc_fw->header_size != sizeof(struct guc_css_header)) {
> > -        DRM_ERROR("CSS header definition mismatch\n");
> > +        DRM_NOTE("CSS header definition mismatch\n");
> >           goto fail;
> >       }
> >
> > @@ -593,7 +593,7 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >
> >       /* now RSA */
> >       if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> > -        DRM_ERROR("RSA key size is bad\n");
> > +        DRM_NOTE("RSA key size is bad\n");
> >           goto fail;
> >       }
> >       guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> > @@ -602,14 +602,14 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >       /* At least, it should have header, uCode and RSA. Size of all
> > three. */
> >       size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
> >       if (fw->size < size) {
> > -        DRM_ERROR("Missing firmware components\n");
> > +        DRM_NOTE("Missing firmware components\n");
> >           goto fail;
> >       }
> >
> >       /* Header and uCode will be loaded to WOPCM. Size of the two. */
> >       size = guc_fw->header_size + guc_fw->ucode_size;
> >       if (size > guc_wopcm_size(to_i915(dev))) {
> > -        DRM_ERROR("Firmware is too large to fit in WOPCM\n");
> > +        DRM_NOTE("Firmware is too large to fit in WOPCM\n");
> >           goto fail;
> >       }
> >
> > @@ -624,7 +624,7 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >
> >       if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> >           guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> > -        DRM_ERROR("GuC firmware version %d.%d, required %d.%d\n",
> > +        DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> >               guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> >               guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> >           err = -ENOEXEC;
> > @@ -654,10 +654,10 @@ static void guc_fw_fetch(struct drm_device *dev,
> > struct intel_guc_fw *guc_fw)
> >       return;
> >
> >   fail:
> > +    DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> > +         guc_fw->guc_fw_path, err);
> >       DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj
> > %p\n",
> >           err, fw, guc_fw->guc_fw_obj);
>
> As commented before, I would tidy these two into one while dropping the
> uninteresting debug like object pointe and fw name pointer.

Can't be done. User/admin doesn't want to see internal details, 
developer needs them. The pointers are important not so much for their 
specific values but for NULL/non-NULL -- for debugging we like to know 
whether we hit a lookup error or an allocation failure, or something 
else. So two different messages for two different audiences.

.Dave.

> > -    DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
> > -          guc_fw->guc_fw_path, err);
> >
> >       mutex_lock(&dev->struct_mutex);
> >       obj = guc_fw->guc_fw_obj;
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Also, you can keep it if you decide to act on my suggestions from above.
>
> Regards,
> Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx