[Intel-gfx] [PATCH] drm/i915/pcode: Wait 10 seconds for pcode to settle

[Rodrigo Vivi]

On Wed, Jan 11, 2023 at 04:39:36PM +0100, Andi Shyti wrote:
> Hi Rodrigo,
> 
> On Wed, Jan 11, 2023 at 10:25:56AM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 11, 2023 at 11:44:47AM +0100, Andi Shyti wrote:
> > > From: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
> > > 
> > > During module load not all the punit transaction have completed
> > > and we might end up timing out, as shown by the following
> > > warning:
> > > 
> > >    i915 0000:4d:00.0: drm_WARN_ON_ONCE(timeout_base_ms > 3)
> > > 
> > > Wait 10 seconds for the punit to settle and complete any
> > > outstanding transactions upon module load.
> > 
> > 10 *SECONDS* ?!
> 
> Don't be alarmed :)
> 
> It's up to 10 seconds, otherwise we would end up waiting up to 3
> minutes.
> 
> And I've seen a version (and you did as well) where those 3
> minutes were raised to 6 (for the PVC particular case).

Oh yeap! and that case is funny! Because the indication from PCODE
is that 10 seconds is enough, but there are some rare cases where
it gets stuck and end up taking a very long time. Then they multiplied
the bad rare case to 3, and no idea why how that become 6.

But anyway, thanks for refreshing my memory. When I first noticed this
patch I thought it was in all the platforms, where this wouldn't make
sense. But on discrete where the pcode needs to bring the memory and
gt up before we can really use it, it makes sense.

And then I noticed that your patch is indeed for dgfx only, so it
would be okay. And 10 seconds is okay.

However I also noticed that you do this before the other pcode_init
check that we were told by pcode folks to use. So, I don't understand
how your patch is helping now... you wait for 10 seconds and then you
will wait more 10 seconds on the pcode_ready... why the pcode_ready
check that we have in case already doesn't cover yours?

And why that return?

> 
> Thanks for checking this,
> Andi
> 
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7814
> > > 
> > > Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
> > > Co-developed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pcode.c | 35 ++++++++++++++++++++++++++----
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c
> > > index a234d9b4ed14..3db2ba439bb5 100644
> > > --- a/drivers/gpu/drm/i915/intel_pcode.c
> > > +++ b/drivers/gpu/drm/i915/intel_pcode.c
> > > @@ -204,15 +204,42 @@ int skl_pcode_request(struct intel_uncore *uncore, u32 mbox, u32 request,
> > >  #undef COND
> > >  }
> > >  
> > > +static int pcode_init_wait(struct intel_uncore *uncore, int timeout_ms)
> > > +{
> > > +	if (__intel_wait_for_register_fw(uncore,
> > > +					 GEN6_PCODE_MAILBOX,
> > > +					 GEN6_PCODE_READY, 0,
> > > +					 500, timeout_ms,
> > > +					 NULL))
> > > +		return -EPROBE_DEFER;
> > > +
> > > +	return skl_pcode_request(uncore,
> > > +				 DG1_PCODE_STATUS,
> > > +				 DG1_UNCORE_GET_INIT_STATUS,
> > > +				 DG1_UNCORE_INIT_STATUS_COMPLETE,
> > > +				 DG1_UNCORE_INIT_STATUS_COMPLETE, timeout_ms);
> > > +}
> > > +
> > >  int intel_pcode_init(struct intel_uncore *uncore)
> > >  {
> > > +	int err;
> > > +
> > >  	if (!IS_DGFX(uncore->i915))
> > >  		return 0;
> > >  
> > > -	return skl_pcode_request(uncore, DG1_PCODE_STATUS,
> > > -				 DG1_UNCORE_GET_INIT_STATUS,
> > > -				 DG1_UNCORE_INIT_STATUS_COMPLETE,
> > > -				 DG1_UNCORE_INIT_STATUS_COMPLETE, 180000);
> > > +	/*
> > > +	 * Wait 10 seconds so that the punit to settle and complete
> > > +	 * any outstanding transactions upon module load
> > > +	 */
> > > +	err = pcode_init_wait(uncore, 10000);
> > > +
> > > +	if (err) {
> > > +		drm_notice(&uncore->i915->drm,
> > > +			   "Waiting for HW initialisation...\n");
> > > +		err = pcode_init_wait(uncore, 180000);
> > > +	}
> > > +
> > > +	return err;
> > >  }
> > >  
> > >  int snb_pcode_read_p(struct intel_uncore *uncore, u32 mbcmd, u32 p1, u32 p2, u32 *val)
> > > -- 
> > > 2.34.1
> > >