[PATCH v2 1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[PATCH v2 1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[Joonas Lahtinen]

"BIT(max) - 1" will overflow when max = 32, and GCC will complain.
We already have GENMASK for generating the mask, use it!

v2: Majestic off by one spotted (Chris)

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_device_info.c | 2 +-
 drivers/gpu/drm/i915/intel_fbdev.c       | 2 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index fcf8181..0891cc0 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
 	 * The subslice disable field is global, i.e. it applies
 	 * to each of the enabled slices.
 	 */
-	sseu->subslice_mask = BIT(ss_max) - 1;
+	sseu->subslice_mask = GENMASK(ss_max - 1, 0);
 	sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
 				 GEN8_F2_SS_DIS_SHIFT);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 281c5c4..e6f3eb2d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		return false;
 
 	memcpy(save_enabled, enabled, count);
-	mask = BIT(count) - 1;
+	mask = GENMASK(count - 1, 0);
 	conn_configured = 0;
 retry:
 	for (i = 0; i < count; i++) {
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 66aa1bb..94df466 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1249,7 +1249,7 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 	vlv_set_power_well(dev_priv, power_well, false);
 }
 
-#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
+#define POWER_DOMAIN_MASK (GENMASK(POWER_DOMAIN_NUM - 1, 0))
 
 static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,
 						 int power_well_id)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/2] drm/i915: Use for_each_power_domain() in i915_power_domain_info()

[Joonas Lahtinen]

Macro seems to do exactly the same thing.

Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 152f23d..6400f83 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2822,15 +2822,10 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
 		seq_printf(m, "%-25s %d\n", power_well->name,
 			   power_well->count);
 
-		for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
-		     power_domain++) {
-			if (!(BIT(power_domain) & power_well->domains))
-				continue;
-
+		for_each_power_domain(power_domain, power_well->domains)
 			seq_printf(m, "  %-23s %d\n",
 				 intel_display_power_domain_str(power_domain),
 				 power_domains->domain_use_count[power_domain]);
-		}
 	}
 
 	mutex_unlock(&power_domains->lock);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Use for_each_power_domain() in i915_power_domain_info()

[Chris Wilson]

On Wed, Feb 08, 2017 at 03:12:10PM +0200, Joonas Lahtinen wrote:
> Macro seems to do exactly the same thing.

And a chance there for BIT()!
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Use for_each_power_domain() in i915_power_domain_info()

[Joonas Lahtinen]

On ke, 2017-02-08 at 13:39 +0000, Chris Wilson wrote:
> On Wed, Feb 08, 2017 at 03:12:10PM +0200, Joonas Lahtinen wrote:
> > 
> > Macro seems to do exactly the same thing.
> 
> And a chance there for BIT()!
> > 
> > 
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed, thanks for the review.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[Chris Wilson]

On Wed, Feb 08, 2017 at 03:12:09PM +0200, Joonas Lahtinen wrote:
> "BIT(max) - 1" will overflow when max = 32, and GCC will complain.
> We already have GENMASK for generating the mask, use it!
> 
> v2: Majestic off by one spotted (Chris)
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2,1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)
URL   : https://patchwork.freedesktop.org/series/19308/
State : failure

== Summary ==

Series 19308v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/19308/revisions/1/mbox/

Test gem_exec_fence:
        Subgroup basic-await-default:
                pass       -> INCOMPLETE (fi-ilk-650)

fi-bdw-5557u     total:252  pass:238  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:230  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:47   pass:26   dwarn:0   dfail:0   fail:0   skip:20 
fi-ivb-3520m     total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:252  pass:231  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:252  pass:229  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:252  pass:232  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:252  pass:227  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:252  pass:239  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:252  pass:220  dwarn:0   dfail:0   fail:0   skip:32 

13888993e891a50d0620ceb2a699fc2309b8fe2b drm-tip: 2017y-02m-08d-13h-33m-36s UTC integration manifest
677712e drm/i915: Use for_each_power_domain() in i915_power_domain_info()
668acb1 drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3736/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[Paulo Zanoni]

Em Qua, 2017-02-08 às 15:12 +0200, Joonas Lahtinen escreveu:
> "BIT(max) - 1" will overflow when max = 32, and GCC will complain.
> We already have GENMASK for generating the mask, use it!
> 
> v2: Majestic off by one spotted (Chris)
> 
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_device_info.c | 2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c       | 2 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> index fcf8181..0891cc0 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct
> drm_i915_private *dev_priv)
>  	 * The subslice disable field is global, i.e. it applies
>  	 * to each of the enabled slices.
>  	 */
> -	sseu->subslice_mask = BIT(ss_max) - 1;
> +	sseu->subslice_mask = GENMASK(ss_max - 1, 0);
>  	sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
>  				 GEN8_F2_SS_DIS_SHIFT);
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 281c5c4..e6f3eb2d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct
> drm_fb_helper *fb_helper,
>  		return false;
>  
>  	memcpy(save_enabled, enabled, count);
> -	mask = BIT(count) - 1;
> +	mask = GENMASK(count - 1, 0);

Due to some debugging accident I ended up with a machine where count is
zero.

In this case:

"BIT(count) - 1" is 0
"GENMASK(count - 1, 0)" is 0xFFFFFFFFFFFFFFFF

The consequence is that the machine freezes after i915.ko is loaded.

So we have a "short blanket" problem here: one solution is wrong for
the maximum value while the other solution is wrong for the minimum
value. Either your chest or your feet gets warm, not both.

I see that "count" comes from fb_helper->connector_count. The
drm_fb_helper.h documentation says that this is "number of connected
connectors". So now I'm wondering that maybe zero is actually a
possible value (outside of my accident), in which case this patch would
be considered a regression. Maybe for those PCH_NONE/PCH_NOP cases?

Perhaps we could only revert this specific chunk and keep the power
domain chunk using GENMASK? Any other solutions?

Also, I have no idea if the subslice_mask case would accept zero as
input. I'll let you do the analysis of this piece of the code.

Thanks,
Paulo

>  	conn_configured = 0;
>  retry:
>  	for (i = 0; i < count; i++) {
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 66aa1bb..94df466 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1249,7 +1249,7 @@ static void
> vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>  	vlv_set_power_well(dev_priv, power_well, false);
>  }
>  
> -#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
> +#define POWER_DOMAIN_MASK (GENMASK(POWER_DOMAIN_NUM - 1, 0))
>  
>  static struct i915_power_well *lookup_power_well(struct
> drm_i915_private *dev_priv,
>  						 int power_well_id)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915: Avoid BIT(max) - 1 and use GENMASK(max - 1, 0)

[Chris Wilson]

On Fri, Feb 24, 2017 at 06:03:09PM -0300, Paulo Zanoni wrote:
> Em Qua, 2017-02-08 às 15:12 +0200, Joonas Lahtinen escreveu:
> > "BIT(max) - 1" will overflow when max = 32, and GCC will complain.
> > We already have GENMASK for generating the mask, use it!
> > 
> > v2: Majestic off by one spotted (Chris)
> > 
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_device_info.c | 2 +-
> >  drivers/gpu/drm/i915/intel_fbdev.c       | 2 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c  | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > b/drivers/gpu/drm/i915/intel_device_info.c
> > index fcf8181..0891cc0 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -234,7 +234,7 @@ static void broadwell_sseu_info_init(struct
> > drm_i915_private *dev_priv)
> >  	 * The subslice disable field is global, i.e. it applies
> >  	 * to each of the enabled slices.
> >  	 */
> > -	sseu->subslice_mask = BIT(ss_max) - 1;
> > +	sseu->subslice_mask = GENMASK(ss_max - 1, 0);
> >  	sseu->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
> >  				 GEN8_F2_SS_DIS_SHIFT);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> > b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 281c5c4..e6f3eb2d 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -369,7 +369,7 @@ static bool intel_fb_initial_config(struct
> > drm_fb_helper *fb_helper,
> >  		return false;
> >  
> >  	memcpy(save_enabled, enabled, count);
> > -	mask = BIT(count) - 1;
> > +	mask = GENMASK(count - 1, 0);
> 
> Due to some debugging accident I ended up with a machine where count is
> zero.
> 
> In this case:
> 
> "BIT(count) - 1" is 0
> "GENMASK(count - 1, 0)" is 0xFFFFFFFFFFFFFFFF
> 
> The consequence is that the machine freezes after i915.ko is loaded.
> 
> So we have a "short blanket" problem here: one solution is wrong for
> the maximum value while the other solution is wrong for the minimum
> value. Either your chest or your feet gets warm, not both.
> 
> I see that "count" comes from fb_helper->connector_count. The
> drm_fb_helper.h documentation says that this is "number of connected
> connectors". So now I'm wondering that maybe zero is actually a
> possible value (outside of my accident), in which case this patch would
> be considered a regression. Maybe for those PCH_NONE/PCH_NOP cases?
> 
> Perhaps we could only revert this specific chunk and keep the power
> domain chunk using GENMASK? Any other solutions?
> 
> Also, I have no idea if the subslice_mask case would accept zero as
> input. I'll let you do the analysis of this piece of the code.

It's already been fixed (hopefully).

commit 754a76591b12c88f57ad8b4ca533a5c9566a1922
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Feb 24 11:43:06 2017 +0000

    drm/i915/fbdev: Stop repeating tile configuration on stagnation

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx