Re: [Intel-gfx] [PATCH v2] drm/i915/pcode: Give the punit time to settle before fatally failing

From: Andrzej Hajda <andrzej.hajda@intel.com>
To: Andi Shyti <andi.shyti@linux.intel.com>,
	intel-gfx@lists.freedesktop.org,
	 dri-devel@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Chris Wilson <chris.p.wilson@linux.intel.com>,
	Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/pcode: Give the punit time to settle before fatally failing
Date: Tue, 7 Feb 2023 10:08:12 +0100	[thread overview]
Message-ID: <d88c8392-ebd5-ffd3-d64a-a461a0f50f53@intel.com> (raw)
In-Reply-To: <20230206183236.109908-1-andi.shyti@linux.intel.com>

On 06.02.2023 19:32, Andi Shyti wrote:
> From: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> 
> During module load the punit might still be busy with its booting
> routines. During this time we try to communicate with it but we
> fail because we don't receive any feedback from it and we return
> immediately with a -EINVAL fatal error.
> 
> At this point the driver load is "dramatically" aborted. The
> following error message notifies us about it.
> 
>     i915 0000:4d:00.0: drm_WARN_ON_ONCE(timeout_base_ms > 3)
> 
> It would be enough to wait a little in order to give the punit
> the chance to come up bright and shiny, ready to interact with
> the driver.
> 
> Wait up 10 seconds for the punit to settle and complete any
> outstanding transactions upon module load. If it still fails try
> again with a longer timeout, 180s, 3 minutes. If it still fails
> then return -EPROBE_DEFER, in order to give the punit a second
> chance.
> 
> Even if these timers might look long, we should consider that the
> punit, depending on the platforms, might need long times to
> complete its routines. Besides we want to try anything possible
> to move forward before deciding to abort the driver's load.
> 
> The issue has been reported in:
> 
>     https://gitlab.freedesktop.org/drm/intel/-/issues/7814
> 
> The changes in this patch are valid only and uniquely during
> boot. The common transactions with the punit during the driver's
> normal operation are not affected.
> 
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> Co-developed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

With improved commit message it looks OK for me. There is still question 
why it takes so long for punit to become ready.
Anyway:
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
> Hi,
> 
> I'm proposing again the same patch as v1 with a hopefully more
> descriptive commit log in order to minimize the
> misunderstandings that we had during the v1 review.
> 
> Thanks,
> Andi
> 
> Changelog:
> ==========
> v1 -> v2:
>   - write a more descriptive commit log.
>   - add Chris SoB which was triggering a checkpatch error.
> 
>   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 a234d9b4ed143..3db2ba439bb57 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)