On Mon, Jan 30, 2023 at 11:37:29AM -0500, Rodrigo Vivi wrote:
> On Mon, Jan 30, 2023 at 05:12:48PM +0100, Andi Shyti wrote:
> > Hi Rodrigo,
> >
> > > > > In the call flow invoked by intel_pcode_init(), I've added brief comments
> > > > > where further clarification is needed in this scenario, and a description of
> > > > > the suspicious scenario at the bottom.
> > > > >
> > > > > -------------------------------------------------------------------------
> > > > > intel_pcode_init()
> > > > > |
> > > > > +-> skl_pcode_request(uncore, DG1_PCODE_STATUS,
> > > > > DG1_UNCORE_GET_INIT_STATUS,
> > > > > DG1_UNCORE_INIT_STATUS_COMPLETE,
> > > > > DG1_UNCORE_INIT_STATUS_COMPLETE, 180000);
> > > > > |
> > > > > +-> skl_pcode_try_request()
> > > > > |
> > > > > +-> *status = __snb_pcode_rw(uncore, mbox, &request, NULL,
> > > > > 500, 0, true);
> > > > >
> > > > > -------------------------------------------------------------------------
> > > > > static int __snb_pcode_rw(struct intel_uncore *uncore, u32 mbox,
> > > > > u32 *val, u32 *val1,
> > > > > int fast_timeout_us, int slow_timeout_ms,
> > > > > bool is_read)
> > > > > {
> > > > > ...
> > > > > /* Before writing a value to the GEN6_PCODE_DATA register,
> > > > > check if the bit in the GEN6_PCODE_MAILBOX register indicates
> > > > > BUSY. */
> > > > > if (intel_uncore_read_fw(uncore, GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
> > > > > return -EAGAIN;
> > > >
> > > > what if we fail here because the punit is still initializing and
> > > > will be ready, say, in 10 seconds?
> > > >
> > > > GG, without going any further, we fail here! The -EAGAIN we
> > > > receive from the test comes from this point. We don't fail with
> > > > -ETIMEDOUT, but with -EAGAIN and the reason is because the punit
> > > > is not ready to perform the very fist communication and we fail
> > > > the probing.
> > > >
> > > > It doesn't mean, though, that there is anything wrong, we just
> > > > need to wait a bit before "taking drastic decisions"!
> > > >
> > > > This patch is suggesting to wait up to 10s for the punit to be
> > > > ready and eventually try to probe again... and, indeed, it works!
> > >
> > > As GG, what I still don't understand is how this extra 10 seconds
> > > wait helps... have you tried to simple add the 10 to the 180 and
> > > make the code 190 sec instead?
> >
> > maybe I haven't been able to explain the issue properly.
> >
> > I can even set that timer to 2hrs and a half and nothing changes
> > because we fail before.
> >
> > Here it's not a matter of how much do I wait but when do I check
> > the pcode readiness (i.e. signalled by the GEN6_PCODE_READY bit
> > in the GEN6_PCODE_MAILBOX register).
> >
> > During a normal run we are always sure that communicating with
> > the punit works, because we made it sure during the previous
> > transaction.
> >
> > During probe there is no previous transaction and we start
> > communicating with the punit without making sure that it is
> > ready. And indeed some times it is not, so that we suppress the
> > probing on purpose instead of giving it another chance.
> >
> > I admit that the commit message is not written properly and
> > rather misleading, but here it's not at all a matter of how much
> > do I wait.
>
> The commit message was initially confused because it looks like
> we are just adding a wait, without doing anything....
>
> But looking to the code we can see that it will wait until
> pcode is ready with a timeout of 10 seconds.
>
> But if pcode is ready in 10 seconds, why pcode is not ready
> in 190 seconds. We are doing absolutely nothing more that could
> make pcode ready in 10 seconds that won't be in 190.
>
> This is what we are missing here... The code as is doesn't make
> a lot of sense to us and it looks like it is solving the issue
> by the 10 extra seconds and not by some special status checking.
Okay, after an offline talk I am convinced now that we need some
check like this in some place.
But the commit message needs be be fully re-written.
It needs to be clear that underneath, the pcode communication
functions will do a check for ready without any wait, what will
make desired timeout to never really wait for the pcode done
and prematurely return.
at __snb_pcode_rw():
if (intel_uncore_read_fw(uncore, GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
return -EAGAIN;
So, for this reason we need to ensure that pcode is really ready
before we wait.
Other options are to handle the EAGAIN return and then wait.
Or even change the __snb_pcode_rw to ensure that it is ready...
Something like:
- if (intel_uncore_read_fw(uncore, GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
- return -EAGAIN;
+ if (__intel_wait_for_register_fw(uncore, GEN6_PCODE_MAILBOX,
+ GEN6_PCODE_READY, GEN6_PCODE_READY,
+ fast_timeout_us,
+ slow_timeout_ms,
+ NULL))
+ return -ETIMEDOUT;
In the best regular case it will not change anything since the wait
for will return 0 immediatelly... in the worst case we would double
the timeout value, but this would be only in this insane case of
180 seconds anyway and if it goes really really nasty...
But anyway, now the motivation and the source of the issue is pretty
clear, I can add my rv-b there, but I really want a better commit msg
at least...
>
> >
> > Thanks, Rodrigo!
> > Andi
> >
> > > >
> > > > Andi
> > > >
> > > > >
> > > > > /* write value to GEN6_PCODE_DATA register */
> > > > > intel_uncore_write_fw(uncore, GEN6_PCODE_DATA, *val);
> > > > >
> > > > > intel_uncore_write_fw(uncore, GEN6_PCODE_DATA1, val1 ? *val1 : 0);
> > > > >
> > > > > /* In this scenario, the value
> > > > > "DG1_PCODE_STATUS | GEN6_PCODE_READY"
> > > > > is written to the GEN6_PCODE_MAILBOX register,
> > > > > so that the Busy status of the GEN6_PCODE_MAILBOX register
> > > > > can be checked later.
> > > > > (When the value of the GEN6_PCODE_READY bit of the
> > > > > GEN6_PCODE_MAILBOX register changes to 0, the operation can
> > > > > be considered completed.) */
> > > > > intel_uncore_write_fw(uncore,
> > > > > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > > > >
> > > > > /* In this scenario, verify that the BUSY status bit in the
> > > > > GEN6_PCODE_MAILBOX register turns off for up to 500us. */
> > > > > if (__intel_wait_for_register_fw(uncore,
> > > > > GEN6_PCODE_MAILBOX,
> > > > > GEN6_PCODE_READY, 0,
> > > > > fast_timeout_us,
> > > > > slow_timeout_ms,
> > > > > &mbox))
> > > > > return -ETIMEDOUT;
> > > > > /* If there is a failure here, it may be considered that the
> > > > > "DG1_PCODE_STATUS | GEN6_PCODE_READY" operation was not
> > > > > completed within 500us */
> > > > > ...
> > > > > }
> > > > >
> > > > > int skl_pcode_request(struct intel_uncore *uncore, u32 mbox, u32 request,
> > > > > u32 reply_mask, u32 reply, int timeout_base_ms)
> > > > > {
> > > > > u32 status;
> > > > > int ret;
> > > > >
> > > > > mutex_lock(&uncore->i915->sb_lock);
> > > > >
> > > > > #define COND \
> > > > > skl_pcode_try_request(uncore, mbox, request, reply_mask, reply, &status)
> > > > >
> > > > > /* the first trial for skl_pcode_try_request() can return
> > > > > -EAGAIN or -ETIMEDOUT. And the code did not check the error
> > > > > code here, so we don't know how far the __snb_pcode_rw()
> > > > > function went. It is not known whether the pcode_mailbox
> > > > > status was busy before writing the value to the
> > > > > GEN6_PCODE_DATA register or after.*/
> > > > > if (COND) {
> > > > > ret = 0;
> > > > > goto out;
> > > > > }
> > > > >
> > > > > /* In this scenario, skl_pcode_try_request() is invoked every
> > > > > 10us for 180 seconds. When skl_pcode_try_request() returns
> > > > > -EAGAIN and -ETIMEDOUT by _wait_for(),
> > > > > -ETIMEDOUT is returned to a variable ret. */
> > > > >
> > > > > ret = _wait_for(COND, timeout_base_ms * 1000, 10, 10);
> > > > >
> > > > > if (!ret)
> > > > > goto out;
> > > > >
> > > > > /*
> > > > > * The above can time out if the number of requests was low (2 in the
> > > > > * worst case) _and_ PCODE was busy for some reason even after a
> > > > > * (queued) request and @timeout_base_ms delay. As a workaround retry
> > > > > * the poll with preemption disabled to maximize the number of
> > > > > * requests. Increase the timeout from @timeout_base_ms to 50ms to
> > > > > * account for interrupts that could reduce the number of these
> > > > > * requests, and for any quirks of the PCODE firmware that delays
> > > > > * the request completion.
> > > > > */
> > > > > drm_dbg_kms(&uncore->i915->drm,
> > > > > "PCODE timeout, retrying with preemption disabled\n");
> > > > > drm_WARN_ON_ONCE(&uncore->i915->drm, timeout_base_ms > 3);
> > > > > preempt_disable();
> > > > > ret = wait_for_atomic(COND, 50);
> > > > >
> > > > > preempt_enable();
> > > > >
> > > > > out:
> > > > > mutex_unlock(&uncore->i915->sb_lock);
> > > > > return status ? status : ret;
> > > > > #undef COND
> > > > > }
> > > > >
> > > > > ---------------------------------------------------------
> > > > >
> > > > > If you try skl_pcode_try_request() for 180 seconds in skl_pcode_request(),
> > > > > and the first "intel_uncore_read_fw(uncore, GEN6_PCODE_MAILBOX) &
> > > > > GEN6_PCODE_READY)" call in __snb_pcode_rw() that skl_pcode_try_request()
> > > > > invokes always fails. if then it does not make sense to me why this patch
> > > > > fixes it by just waiting 10 seconds.This is because if it was called with
> > > > > the flow described above, 180 seconds is longer than 10 seconds, so the
> > > > > scenario you mentioned is also covered in the existing code.
> > > > >
> > > > > To describe in more detail the second scenario I previously commented on:
> > > > > skl_pcode_request() tries skl_pcode_try_request() for 180 seconds
> > > > > 1) In skl_pcode_try_request(), the GEN6_PCODE_MAILBOX register bit is not
> > > > > BUSY, so the value is written to the GEN6_PCODE_DATA1 register.
> > > > > 2) skl_pcode_try_request() returns -ETIMEDOUT value because the operation
> > > > > of 1) does not complete within 500us.
> > > > > 3) Scenario in which the GEN6_PCODE_MAILBOX register bit is checked as BUSY
> > > > > and returns -EAGAIN in the last call of skl_pcode_try_request() invoked by
> > > > > skl_pcode_request()
> > > > >
> > > > > If the reason why this problem occurred is because of this scenario,
> > > > > shouldn't there be an attempt to increase fast_timeout_us used as an
> > > > > argument of __snb_pcode_rw() to 500us or more when skl_pcode_try_request()
> > > > > returns -ETIMEDOUT?
> > > > >
> > > > > Br,
> > > > > G.G.
