> -----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.
> > + 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?##
ret = 0;
goto out;
}
>
> --Imre
>
> >
> > /**
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
