drm/i915: check status and reply value both in skl_pcode_try_request()

Submitted by Weinan Li on Feb. 22, 2017, 2:25 a.m.

Details

Message ID 1487730344-20125-1-git-send-email-weinan.z.li@intel.com
State New
Headers show
Series "drm/i915: check status and reply value both in skl_pcode_try_request()" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Weinan Li Feb. 22, 2017, 2:25 a.m.
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(-)

Patch hide | download patch | download mbox

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);
 }
 
 /**

Comments

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