[Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.

[Chris Wilson]

Quoting Anusha Srivatsa (2017-11-02 20:28:10)
> On Wed, Nov 01, 2017 at 01:24:15PM +0000, Chris Wilson wrote:
> > Quoting Michal Wajdeczko (2017-11-01 13:14:33)
> > > On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa  
> > > > @@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct  
> > > > drm_i915_private *dev_priv,
> > > >        */
> > > >       ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> > > > +     load_time = ktime_ms_delta(ktime_get(), start_load);
> > > > +
> > > >       DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> > > >                       I915_READ(DMA_CTRL), status);
> > > >       if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > > >               DRM_ERROR("GuC firmware signature verification failed\n");
> > > >               ret = -ENOEXEC;
> > > > -     }
> > > > +     } else if (load_time > 20)
> > > > +             DRM_NOTE("GuC load takes more than acceptable threshold\n");
> > > 
> > > Please add threshold and actual load time to the message to let user
> > > know that times
> > 
> > The more important question is why are we telling the user this at all;
> > a significant but normal condition. What do we expect them to do? It
> > doesn't impair any functionality of the driver, it just took longer than
> > you expected -- which may be simply because the system was doing
> > something else and we slept for longer.
> 
> Chris, I am inclining to have this info more for us than the user. It is more of
> a debug print to give us some data. We can see if firmware takes peculiarly
> long time to load. We know its normal to be within 20ms range. So, alert
> if it takes longer than that...

Sure, but the impact is that this is a user facing message, even marked
as a significant message. We are quite capable of parsing debug
messages; even capable of setting up ftrace to extract this timing info
without adding the dmesg in the first place...
-Chris