Re: [PATCH v2] drm/xe: Change pcode timeout to 50msec while polling again

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, May 08, 2024 at 07:55:10AM -0500, Lucas De Marchi wrote:
> On Wed, May 08, 2024 at 09:13:28AM GMT, Himal Prasad Ghimiray wrote:
> > Polling is initially attempted with timeout_base_ms enabled for
> > preemption, and if it exceeds this timeframe, another attempt is made
> > without preemption, allowing an additional 50 ms before timing out.
> > 
> > Display driver can request for timeout of 3 msec, hence modify the
> > warning condition.
> 
> this seems like a separate fix?
> 
> afaics below we have:
> 
> 1) fix the warning to consider the display case ... and shouldn't the
>    warning be before the first pcode_try_request()? Why do we want to
>    warn before a fixed-duration rather than before the variable duration
>    passed as argument?
> 
> 2) change the second timeout to be a fixed 50 msec rather than 1msec so
>    it follows what was actually documented in that comment.

(for the 1 and 2 I was okay with having the squashed patch tbh, but
yeap, probably better a separate one)

3) status = 0;
(this was not even mentioned in the commit message, and I noticed
after the fact when I was about to merge it. Sorry about that.)

> 
> or maybe I'm missing something here... could you clarify?
> 
> Lucas De Marchi
> 
> > 
> > v2
> > - Rebase
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_pcode.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> > index c010ef16fbf5..2771eed1e45f 100644
> > --- a/drivers/gpu/drm/xe/xe_pcode.c
> > +++ b/drivers/gpu/drm/xe/xe_pcode.c
> > @@ -10,6 +10,7 @@
> > 
> > #include <drm/drm_managed.h>
> > 
> > +#include "xe_assert.h"
> > #include "xe_device.h"
> > #include "xe_gt.h"
> > #include "xe_mmio.h"
> > @@ -124,6 +125,8 @@ static int pcode_try_request(struct xe_gt *gt, u32 mbox,
> > {
> > 	int slept, wait = 10;
> > 
> > +	xe_gt_assert(gt, timeout_us > 0);
> > +
> > 	for (slept = 0; slept < timeout_us; slept += wait) {
> > 		if (locked)
> > 			*status = pcode_mailbox_rw(gt, mbox, &request, NULL, 1, true,
> > @@ -166,7 +169,7 @@ static int pcode_try_request(struct xe_gt *gt, u32 mbox,
> > int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
> > 		      u32 reply_mask, u32 reply, int timeout_base_ms)
> > {
> > -	u32 status;
> > +	u32 status = 0;
> > 	int ret;
> > 
> > 	mutex_lock(&gt->pcode.lock);
> > @@ -188,10 +191,10 @@ int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
> > 	 */
> > 	drm_err(&gt_to_xe(gt)->drm,
> > 		"PCODE timeout, retrying with preemption disabled\n");
> > -	drm_WARN_ON_ONCE(&gt_to_xe(gt)->drm, timeout_base_ms > 1);
> > +	drm_WARN_ON_ONCE(&gt_to_xe(gt)->drm, timeout_base_ms > 3);
> > 	preempt_disable();
> > 	ret = pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
> > -				true, timeout_base_ms * 1000, true);
> > +				true, 50 * 1000, true);
> > 	preempt_enable();
> > 
> > out:
> > -- 
> > 2.25.1
> >