Thanks Imre. I see, it's a little hard to read, need to check the final state across 2 level function return value.
Thanks.
Best Regards.
Weinan, LI
> -----Original Message-----
> From: Deak, Imre
> Sent: Wednesday, February 22, 2017 4:56 PM
> To: Li, Weinan Z <weinan.z.li@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/i915: check status and reply value both
> in skl_pcode_try_request()
>
> On Wed, Feb 22, 2017 at 10:17:21AM +0200, Li, Weinan Z wrote:
> > > -----Original Message-----
> > > From: Deak, Imre
> > > Sent: Wednesday, February 22, 2017 3:54 PM
> > > To: Li, Weinan Z <weinan.z.li@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH] drm/i915: check status and reply
> > > value both in skl_pcode_try_request()
> > >
> > > On Wed, Feb 22, 2017 at 10:25:44AM +0800, Weinan Li wrote:
> > > > skl_pcode_try_request() call sandybridge_pcode_read(), check both
> > > > return status and value simultanously, ensure it got correct value
> > > > without
> > > error.
> > > >
> > > > Signed-off-by: Weinan Li <weinan.z.li@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c index ae2c0bb..e7b12ec 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -7882,7 +7882,7 @@ static bool skl_pcode_try_request(struct
> > > > drm_i915_private *dev_priv, u32 mbox,
> > > >
> > > > *status = sandybridge_pcode_read(dev_priv, mbox, &val);
> > > >
> > > > - return *status || ((val & reply_mask) == reply);
> > *status == 0 means success, otherwise error happened.
>
> Yes.
>
> > > > + return (!*status) && ((val & reply_mask) == reply);
> > > > }
> > >
> > > The original looks ok to me. The condition becomes true if PCODE
> > > reports an error in *status or we get the expected reply. *status is
> > > then rechecked in skl_pcode_request().
> > int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32
> request,
> > u32 reply_mask, u32 reply, int timeout_base_ms) {
> > u32 status;
> > int ret;
> >
> > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >
> > #define COND skl_pcode_try_request(dev_priv, mbox, request,
> reply_mask, reply, \
> > &status)
> >
> > /*
> > * Prime the PCODE by doing a request first. Normally it guarantees
> > * that a subsequent request, at most @timeout_base_ms later,
> succeeds.
> > * _wait_for() doesn't guarantee when its passed condition is
> evaluated
> > * first, so send the first request explicitly.
> > */
> > if (COND) {##here will deal as success although pcode_read() get
> error happened.
> > Is this expected behavior?##
>
> It's not regarded as success, it's regarded as a condition to end the polling.
> That is either a PCODE error returned in status or the expected reply
> received (matching reply_mask/reply). status is rechecked at the end of the
> function.
>
> --Imre
>
> > ret = 0;
> > goto out;
> > }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
