[Intel-gfx] [PATCH] drm/i915: check status and reply value both in skl_pcode_try_request()

[Li, Weinan Z]

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 at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 at intel.com>
> > > Cc: intel-gfx at lists.freedesktop.org;
> > > intel-gvt-dev at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [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 at intel.com>
> > > > ---
> > > >  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;
> > 	}