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

[Imre Deak]

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;
> 	}