| Message ID | 1487730344-20125-1-git-send-email-weinan.z.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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); > + 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(). --Imre > > /** > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Deak, Imre > Sent: Wednesday, February 22, 2017 3:54 PM > To: Li, Weinan Z <weinan.z.li@intel.com> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@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@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. > > + 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@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@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; > }
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@intel.com> > Cc: intel-gfx@lists.freedesktop.org; intel-gvt-dev@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@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; > > > intel-gvt-dev@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@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; > > }
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); + return (!*status) && ((val & reply_mask) == reply); } /**
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@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)