diff mbox

drm/i915/guc: Remove one unnecessary variable

Message ID 1466513303-24702-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin June 21, 2016, 12:48 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

No need for local struct drm_device * since dev_priv is the
correct thing to pass in to NEEDS_WaRsDisableCoarsePowerGating
anyway. Changed the macro definition for the latter to reflect
that as well.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 7 ++++---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Chris Wilson June 21, 2016, 1 p.m. UTC | #1 
On Tue, Jun 21, 2016 at 01:48:23PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> No need for local struct drm_device * since dev_priv is the
> correct thing to pass in to NEEDS_WaRsDisableCoarsePowerGating
> anyway. Changed the macro definition for the latter to reflect
> that as well.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 7 ++++---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 48928227bdcc..3775d26ac573 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2788,9 +2788,10 @@ struct drm_i915_cmd_table {
>  #define HAS_BROKEN_CS_TLB(dev)		(IS_I830(dev) || IS_845G(dev))
>  
>  /* WaRsDisableCoarsePowerGating:skl,bxt */
> -#define NEEDS_WaRsDisableCoarsePowerGating(dev) (IS_BXT_REVID(dev, 0, BXT_REVID_A1) || \
> -						 IS_SKL_GT3(dev) || \
> -						 IS_SKL_GT4(dev))
> +#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) (IS_BXT_REVID(dev_priv, \
> +						     0, BXT_REVID_A1) || \
> +						     IS_SKL_GT3(dev_priv) || \
> +						     IS_SKL_GT4(dev_priv))

#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
	(IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1) || \
	 IS_SKL_GT3(dev_priv) ||
	 IS_SKL_GT4(dev_priv))

Other than that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Just wondering if you fancy having a go at IS_SKL_GT(dev_priv, min, max)...
-Chris
Daniel Vetter June 21, 2016, 1:11 p.m. UTC | #2 
On Tue, Jun 21, 2016 at 01:48:23PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> No need for local struct drm_device * since dev_priv is the
> correct thing to pass in to NEEDS_WaRsDisableCoarsePowerGating
> anyway. Changed the macro definition for the latter to reflect
> that as well.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 7 ++++---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 48928227bdcc..3775d26ac573 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2788,9 +2788,10 @@ struct drm_i915_cmd_table {
>  #define HAS_BROKEN_CS_TLB(dev)		(IS_I830(dev) || IS_845G(dev))
>  
>  /* WaRsDisableCoarsePowerGating:skl,bxt */
> -#define NEEDS_WaRsDisableCoarsePowerGating(dev) (IS_BXT_REVID(dev, 0, BXT_REVID_A1) || \
> -						 IS_SKL_GT3(dev) || \
> -						 IS_SKL_GT4(dev))
> +#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) (IS_BXT_REVID(dev_priv, \
> +						     0, BXT_REVID_A1) || \
> +						     IS_SKL_GT3(dev_priv) || \
> +						     IS_SKL_GT4(dev_priv))
>  
>  /*
>   * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 22a55ac4e51c..643bc3b2e3b8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -153,12 +153,11 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
>  				     struct i915_guc_client *client)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct drm_device *dev = dev_priv->dev;
>  	u32 data[2];
>  
>  	data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE;
>  	/* WaRsDisableCoarsePowerGating:skl,bxt */
> -	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev))
> +	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
>  		data[1] = 0;
>  	else
>  		/* bit 0 and 1 are for Render and Media domain separately */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tvrtko Ursulin June 21, 2016, 2:02 p.m. UTC | #3 
On 21/06/16 14:00, Chris Wilson wrote:
> On Tue, Jun 21, 2016 at 01:48:23PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> No need for local struct drm_device * since dev_priv is the
>> correct thing to pass in to NEEDS_WaRsDisableCoarsePowerGating
>> anyway. Changed the macro definition for the latter to reflect
>> that as well.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h            | 7 ++++---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 48928227bdcc..3775d26ac573 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2788,9 +2788,10 @@ struct drm_i915_cmd_table {
>>   #define HAS_BROKEN_CS_TLB(dev)		(IS_I830(dev) || IS_845G(dev))
>>
>>   /* WaRsDisableCoarsePowerGating:skl,bxt */
>> -#define NEEDS_WaRsDisableCoarsePowerGating(dev) (IS_BXT_REVID(dev, 0, BXT_REVID_A1) || \
>> -						 IS_SKL_GT3(dev) || \
>> -						 IS_SKL_GT4(dev))
>> +#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) (IS_BXT_REVID(dev_priv, \
>> +						     0, BXT_REVID_A1) || \
>> +						     IS_SKL_GT3(dev_priv) || \
>> +						     IS_SKL_GT4(dev_priv))
>
> #define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> 	(IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1) || \
> 	 IS_SKL_GT3(dev_priv) ||
> 	 IS_SKL_GT4(dev_priv))
>
> Other than that,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Just wondering if you fancy having a go at IS_SKL_GT(dev_priv, min, max)...

What do you have in mind?

This is a single call site for them BTW. And GCC amazingly does manage 
to merge the two GT tests on its own already. :)

Regards,

Tvrtko
Tvrtko Ursulin June 21, 2016, 2:44 p.m. UTC | #4 
On 21/06/16 15:41, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/guc: Remove one unnecessary variable (rev2)
> URL   : https://patchwork.freedesktop.org/series/8995/
> State : warning
>
> == Summary ==
>
> Series 8995v2 drm/i915/guc: Remove one unnecessary variable
> http://patchwork.freedesktop.org/api/1.0/series/8995/revisions/2/mbox
>
> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-a:
>                  skip       -> DMESG-WARN (ro-bdw-i5-5250u)
>          Subgroup suspend-read-crc-pipe-b:
>                  skip       -> DMESG-WARN (ro-bdw-i5-5250u)

https://bugs.freedesktop.org/show_bug.cgi?id=96614

> fi-hsw-i7-4770k  total:225  pass:192  dwarn:0   dfail:0   fail:2   skip:31
> fi-kbl-qkkr      total:225  pass:158  dwarn:29  dfail:0   fail:2   skip:36
> fi-skl-i5-6260u  total:225  pass:200  dwarn:0   dfail:0   fail:2   skip:23
> fi-snb-i7-2600   total:225  pass:172  dwarn:0   dfail:0   fail:2   skip:51
> ro-bdw-i5-5250u  total:225  pass:197  dwarn:3   dfail:0   fail:0   skip:25
> ro-bdw-i7-5557U  total:225  pass:198  dwarn:0   dfail:0   fail:0   skip:27
> ro-bdw-i7-5600u  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40
> ro-byt-n2820     total:225  pass:173  dwarn:0   dfail:0   fail:3   skip:49
> ro-hsw-i3-4010u  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35
> ro-hsw-i7-4770r  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35
> ro-ilk-i7-620lm  total:225  pass:150  dwarn:0   dfail:0   fail:1   skip:74
> ro-ilk1-i5-650   total:220  pass:150  dwarn:0   dfail:0   fail:1   skip:69
> ro-ivb-i7-3770   total:225  pass:181  dwarn:0   dfail:0   fail:0   skip:44
> ro-ivb2-i7-3770  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40
> ro-skl3-i5-6260u total:225  pass:201  dwarn:1   dfail:0   fail:0   skip:23
> ro-snb-i7-2620M  total:225  pass:174  dwarn:0   dfail:0   fail:1   skip:50
> fi-skl-i7-6700k failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1257/
>
> 9d436e8 drm-intel-nightly: 2016y-06m-21d-13h-57m-22s UTC integration manifest
> 05a0233 drm/i915/guc: Remove one unnecessary variable

Merged to dinq, thanks for the review!

Regards,

Tvrtko
Chris Wilson June 21, 2016, 3:04 p.m. UTC | #5 
On Tue, Jun 21, 2016 at 03:02:57PM +0100, Tvrtko Ursulin wrote:
> 
> On 21/06/16 14:00, Chris Wilson wrote:
> >On Tue, Jun 21, 2016 at 01:48:23PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>No need for local struct drm_device * since dev_priv is the
> >>correct thing to pass in to NEEDS_WaRsDisableCoarsePowerGating
> >>anyway. Changed the macro definition for the latter to reflect
> >>that as well.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Cc: Dave Gordon <david.s.gordon@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h            | 7 ++++---
> >>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
> >>  2 files changed, 5 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 48928227bdcc..3775d26ac573 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -2788,9 +2788,10 @@ struct drm_i915_cmd_table {
> >>  #define HAS_BROKEN_CS_TLB(dev)		(IS_I830(dev) || IS_845G(dev))
> >>
> >>  /* WaRsDisableCoarsePowerGating:skl,bxt */
> >>-#define NEEDS_WaRsDisableCoarsePowerGating(dev) (IS_BXT_REVID(dev, 0, BXT_REVID_A1) || \
> >>-						 IS_SKL_GT3(dev) || \
> >>-						 IS_SKL_GT4(dev))
> >>+#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) (IS_BXT_REVID(dev_priv, \
> >>+						     0, BXT_REVID_A1) || \
> >>+						     IS_SKL_GT3(dev_priv) || \
> >>+						     IS_SKL_GT4(dev_priv))
> >
> >#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) \
> >	(IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1) || \
> >	 IS_SKL_GT3(dev_priv) ||
> >	 IS_SKL_GT4(dev_priv))
> >
> >Other than that,
> >Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >Just wondering if you fancy having a go at IS_SKL_GT(dev_priv, min, max)...
> 
> What do you have in mind?
> 
> This is a single call site for them BTW. And GCC amazingly does
> manage to merge the two GT tests on its own already. :)

#define GT(x) ((INTEL_DEVID(x) & 0xf0) >> 4)
IS_SKL_GT(dev, min, max) ({
	int gt__ = GT(dev);
	IS_SKYLAKE(dev) && gt__ >= (min) && gt__ < (min);
})

Give or take some more massaging.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 48928227bdcc..3775d26ac573 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2788,9 +2788,10 @@  struct drm_i915_cmd_table {
 #define HAS_BROKEN_CS_TLB(dev)		(IS_I830(dev) || IS_845G(dev))
 
 /* WaRsDisableCoarsePowerGating:skl,bxt */
-#define NEEDS_WaRsDisableCoarsePowerGating(dev) (IS_BXT_REVID(dev, 0, BXT_REVID_A1) || \
-						 IS_SKL_GT3(dev) || \
-						 IS_SKL_GT4(dev))
+#define NEEDS_WaRsDisableCoarsePowerGating(dev_priv) (IS_BXT_REVID(dev_priv, \
+						     0, BXT_REVID_A1) || \
+						     IS_SKL_GT3(dev_priv) || \
+						     IS_SKL_GT4(dev_priv))
 
 /*
  * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 22a55ac4e51c..643bc3b2e3b8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -153,12 +153,11 @@  static int host2guc_sample_forcewake(struct intel_guc *guc,
 				     struct i915_guc_client *client)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct drm_device *dev = dev_priv->dev;
 	u32 data[2];
 
 	data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE;
 	/* WaRsDisableCoarsePowerGating:skl,bxt */
-	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev))
+	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
 		data[1] = 0;
 	else
 		/* bit 0 and 1 are for Render and Media domain separately */