[Intel-gfx] [PATCH] drm/i915: Fix skl_pcode_try_request function

[Lisovskiy, Stanislav]

On Wed, Apr 06, 2022 at 12:51:29AM +0300, Govindapillai, Vinod wrote:
> Hi Stan
> 
> Nice Find! Couple of clarifications, please check inline...
> 
> On Tue, 2022-04-05 at 13:41 +0300, Stanislav Lisovskiy wrote:
> > Currently skl_pcode_try_request function doesn't
> > properly handle return value it gets from
> > snb_pcode_rw, but treats status != 0 as success,
> > returning true, which basically doesn't allow
> > to use retry/timeout mechanisms if PCode happens
> > to be busy and returns EGAIN or some other status
> > code not equal to 0.
> >
> > We saw this on real hw and also tried simulating this
> > by always returning -EAGAIN from snb_pcode_rw for 6 times, which
> > currently will just result in false success, while it should
> > have tried until timeout is reached:
> >
> > [   22.357729] i915 0000:00:02.0: [drm:intel_cdclk_dump_config [i915]] Changing CDCLK to
> > 307200 kHz, VCO 614400 kHz, ref 38400 kHz, bypass 19200 kHz, voltage level 0
> > [   22.357831] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 1
> > [   22.357892] i915 0000:00:02.0: [drm:skl_pcode_request [i915]] Success, exiting
> > [   22.357936] i915 0000:00:02.0: [drm] ERROR Failed to inform PCU about cdclk change (err -11,
> > freq 307200)
> >
> > We see en error because higher level api, still notices that status was wrong,
> > however we still did try only once.
> >
> > We fix it by requiring _both_ the status to be 0 and
> > request/reply match for success(true) and function
> > should return failure(false) if either status turns
> > out to be EAGAIN, EBUSY or whatever or reply/request
> > masks do not match.
> >
> > So now we see this in the logs:
> >
> > [   22.318667] i915 0000:00:02.0: [drm:intel_cdclk_dump_config [i915]] Changing CDCLK to
> > 307200 kHz, VCO 614400 kHz, ref 38400 kHz, bypass 19200 kHz, voltage level 0
> > [   22.318782] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 1
> > [   22.318849] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 2
> > [   22.319006] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 3
> > [   22.319091] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 4
> > [   22.319158] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 5
> > [   22.319224] i915 0000:00:02.0: [drm:__snb_pcode_rw [i915]] Returning EAGAIN retry 6
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pcode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pcode.c b/drivers/gpu/drm/i915/intel_pcode.c
> > index 391a37492ce5..fb6c43e8a02f 100644
> > --- a/drivers/gpu/drm/i915/intel_pcode.c
> > +++ b/drivers/gpu/drm/i915/intel_pcode.c
> > @@ -136,7 +136,7 @@ static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox,
> >  {
> >       *status = __snb_pcode_rw(i915, mbox, &request, NULL, 500, 0, true);
> >
> > -     return *status || ((request & reply_mask) == reply);
> > +     return (*status == 0) && ((request & reply_mask) == reply);
> 
> Here I wonder whether you need to check what sort of __snb_pcode_rw return values need the retry!
> Isn't only ETIMEDOUT need the retry? Other return error codes can be probably be ignored from retry?

Hi Vinod! Thanks for comments.

Well theoretically yes, but in practice I think we would prefer to retry in almost all of the cases.
There are also multiple error codes when you need to retry, such as "EAGAIN", "EBUSY" and probably
some others. Thats is probably why original code also doesn't make a difference.

> 
> And should the, "return ret ? ret : status;" in "skl_pcode_request" be change to "return ret ?
> status : ret;" to reflect the correct error code to calling functions after this change?

I think the logic here is such that ret value is somewhat more important than the status, so 
if ret is something not zero - we always prefer returning ret, for the enduser to know what
was the ret ERROR code.
If ret is 0, then we can "afford" to let the enduser know, what was actually the status.

To me it actually sounds a bit wrong, I think we should get status pointer, like "&status"
and modify it, so that calling site _always_ knows both status and ret, mixing those two
is a dangerous strategy which exactly caused some coder confusion and probably the bug, that
this patch is fixing.

Stan

> 
> 
> >  }
> >
> >  /**