diff mbox

[5/5] drm/i915: Warn on obsolete revision checks

Message ID 20180608134205.6452-5-mika.kuoppala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala June 8, 2018, 1:42 p.m. UTC 
If we are doing revision checks against a preproduction
range, when there is already a product, it is a sign
that there is code to be removed.

Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_chipset.h | 30 +++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

Comments

Chris Wilson June 8, 2018, 1:51 p.m. UTC | #1 
Quoting Mika Kuoppala (2018-06-08 14:42:05)
> If we are doing revision checks against a preproduction
> range, when there is already a product, it is a sign
> that there is code to be removed.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_chipset.h | 30 +++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_chipset.h b/drivers/gpu/drm/i915/intel_chipset.h
> index 946c889c0118..bc9ff02dc8df 100644
> --- a/drivers/gpu/drm/i915/intel_chipset.h
> +++ b/drivers/gpu/drm/i915/intel_chipset.h
> @@ -131,6 +131,12 @@
>  #define IS_PREPRODUCTION_HW(dev_priv)   (INTEL_REVID(dev_priv) < FIRST_PRODUCT_REVID(INTEL_INFO(dev_priv)))
>  #define IS_PLATFORM_SUPPORT_ALPHA(intel_info) (FIRST_PRODUCT_REVID(intel_info) == PRODUCT_REVID_UNKNOWN)
>  
> +#define BUILD_BUG_ON_REVID_LT(revid, production_revid) ({ \

BUILD_BUG_ON_PREPRODUCTION()

It doesn't look that general, or widely useful to say REVID_LT. Sort of
implies we will have REVID_GTE etc later.

> +               BUILD_BUG_ON((production_revid) != PRODUCT_REVID_UNKNOWN && \
> +                            (revid) < (production_revid)); \

I'd prefer (!BUILD_BUG_ON_ZERO()) (Or push the
!BUILD_BUG_ON_PREPRODUCTION to the caller as that's easier to read).

That avoids the ({block}) making it less likely to cause problems.
-Chris
Jani Nikula June 11, 2018, 12:26 p.m. UTC | #2 
On Fri, 08 Jun 2018, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> If we are doing revision checks against a preproduction
> range, when there is already a product, it is a sign
> that there is code to be removed.
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_chipset.h | 30 +++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_chipset.h b/drivers/gpu/drm/i915/intel_chipset.h
> index 946c889c0118..bc9ff02dc8df 100644
> --- a/drivers/gpu/drm/i915/intel_chipset.h
> +++ b/drivers/gpu/drm/i915/intel_chipset.h
> @@ -131,6 +131,12 @@
>  #define IS_PREPRODUCTION_HW(dev_priv)   (INTEL_REVID(dev_priv) < FIRST_PRODUCT_REVID(INTEL_INFO(dev_priv)))
>  #define IS_PLATFORM_SUPPORT_ALPHA(intel_info) (FIRST_PRODUCT_REVID(intel_info) == PRODUCT_REVID_UNKNOWN)
>  
> +#define BUILD_BUG_ON_REVID_LT(revid, production_revid) ({ \
> +		BUILD_BUG_ON((production_revid) != PRODUCT_REVID_UNKNOWN && \
> +			     (revid) < (production_revid)); \
> +		1; \
> +	})
> +
>  #define SKL_REVID_A0		0x0
>  #define SKL_REVID_B0		0x1
>  #define SKL_REVID_C0		0x2
> @@ -141,7 +147,9 @@
>  #define SKL_REVID_PRODUCT	SKL_REVID_G0
>  #define SKL_REVID_H0		0x7
>  
> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
> +#define IS_SKL_REVID(p, since, until) \
> +	(BUILD_BUG_ON_REVID_LT(until, SKL_REVID_PRODUCT) && \
> +	 IS_SKYLAKE(p) && IS_REVID(p, since, until))
>  
>  #define BXT_REVID_A0		0x0
>  #define BXT_REVID_A1		0x1
> @@ -151,7 +159,8 @@
>  #define BXT_REVID_PRODUCT	BXT_REVID_C0
>  
>  #define IS_BXT_REVID(dev_priv, since, until) \
> -	(IS_BROXTON(dev_priv) && IS_REVID(dev_priv, since, until))
> +	(BUILD_BUG_ON_REVID_LT(until, BXT_REVID_PRODUCT) && \
> +	 IS_BROXTON(dev_priv) && IS_REVID(dev_priv, since, until))
>  
>  #define KBL_REVID_A0		0x0
>  #define KBL_REVID_B0		0x1
> @@ -161,29 +170,36 @@
>  #define KBL_REVID_E0		0x4
>  
>  #define IS_KBL_REVID(dev_priv, since, until) \
> -	(IS_KABYLAKE(dev_priv) && IS_REVID(dev_priv, since, until))
> +	(BUILD_BUG_ON_REVID_LT(until, KBL_REVID_PRODUCT) && \
> +	 IS_KABYLAKE(dev_priv) && IS_REVID(dev_priv, since, until))
>  
>  #define GLK_REVID_A0		0x0
>  #define GLK_REVID_A1		0x1
> +#define GLK_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
>  
> -#define IS_GLK_REVID(dev_priv, since, until) \
> -	(IS_GEMINILAKE(dev_priv) && IS_REVID(dev_priv, since, until))
> +#define IS_GLK_REVID(dev_priv, since, until)		    \
> +	(BUILD_BUG_ON_REVID_LT(until, GLK_REVID_PRODUCT) && \
> +	 IS_GEMINILAKE(dev_priv) && IS_REVID(dev_priv, since, until))
>  
>  #define CNL_REVID_A0		0x0
>  #define CNL_REVID_B0		0x1
>  #define CNL_REVID_C0		0x2
> +#define CNL_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
>  
>  #define IS_CNL_REVID(p, since, until) \
> -	(IS_CANNONLAKE(p) && IS_REVID(p, since, until))
> +	(BUILD_BUG_ON_REVID_LT(until, CNL_REVID_PRODUCT) && \
> +	 IS_CANNONLAKE(p) && IS_REVID(p, since, until))
>  
>  #define ICL_REVID_A0		0x0
>  #define ICL_REVID_A2		0x1
>  #define ICL_REVID_B0		0x3
>  #define ICL_REVID_B2		0x4
>  #define ICL_REVID_C0		0x5
> +#define ICL_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
>  
>  #define IS_ICL_REVID(p, since, until) \
> -	(IS_ICELAKE(p) && IS_REVID(p, since, until))
> +	(BUILD_BUG_ON_REVID_LT(until, ICL_REVID_PRODUCT) && \
> +	IS_ICELAKE(p) && IS_REVID(p, since, until))

The trouble with this is that it ties the macros *_REVID_PRODUCT to
having to get rid of all the pre-production revid checks. We'll
typically know the product revid *before* we can drop all the
checks. And we can't drop all the checks because there'll be plenty of
hardware to phase out and replace, and it doesn't happen overnight. But
we'd still like to start complaining about the pre-pro hardware use as
soon as possible.

BR,
Jani.

>  
>  /*
>   * The genX designation typically refers to the render engine, so render
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_chipset.h b/drivers/gpu/drm/i915/intel_chipset.h
index 946c889c0118..bc9ff02dc8df 100644
--- a/drivers/gpu/drm/i915/intel_chipset.h
+++ b/drivers/gpu/drm/i915/intel_chipset.h
@@ -131,6 +131,12 @@ 
 #define IS_PREPRODUCTION_HW(dev_priv)   (INTEL_REVID(dev_priv) < FIRST_PRODUCT_REVID(INTEL_INFO(dev_priv)))
 #define IS_PLATFORM_SUPPORT_ALPHA(intel_info) (FIRST_PRODUCT_REVID(intel_info) == PRODUCT_REVID_UNKNOWN)
 
+#define BUILD_BUG_ON_REVID_LT(revid, production_revid) ({ \
+		BUILD_BUG_ON((production_revid) != PRODUCT_REVID_UNKNOWN && \
+			     (revid) < (production_revid)); \
+		1; \
+	})
+
 #define SKL_REVID_A0		0x0
 #define SKL_REVID_B0		0x1
 #define SKL_REVID_C0		0x2
@@ -141,7 +147,9 @@ 
 #define SKL_REVID_PRODUCT	SKL_REVID_G0
 #define SKL_REVID_H0		0x7
 
-#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until))
+#define IS_SKL_REVID(p, since, until) \
+	(BUILD_BUG_ON_REVID_LT(until, SKL_REVID_PRODUCT) && \
+	 IS_SKYLAKE(p) && IS_REVID(p, since, until))
 
 #define BXT_REVID_A0		0x0
 #define BXT_REVID_A1		0x1
@@ -151,7 +159,8 @@ 
 #define BXT_REVID_PRODUCT	BXT_REVID_C0
 
 #define IS_BXT_REVID(dev_priv, since, until) \
-	(IS_BROXTON(dev_priv) && IS_REVID(dev_priv, since, until))
+	(BUILD_BUG_ON_REVID_LT(until, BXT_REVID_PRODUCT) && \
+	 IS_BROXTON(dev_priv) && IS_REVID(dev_priv, since, until))
 
 #define KBL_REVID_A0		0x0
 #define KBL_REVID_B0		0x1
@@ -161,29 +170,36 @@ 
 #define KBL_REVID_E0		0x4
 
 #define IS_KBL_REVID(dev_priv, since, until) \
-	(IS_KABYLAKE(dev_priv) && IS_REVID(dev_priv, since, until))
+	(BUILD_BUG_ON_REVID_LT(until, KBL_REVID_PRODUCT) && \
+	 IS_KABYLAKE(dev_priv) && IS_REVID(dev_priv, since, until))
 
 #define GLK_REVID_A0		0x0
 #define GLK_REVID_A1		0x1
+#define GLK_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
 
-#define IS_GLK_REVID(dev_priv, since, until) \
-	(IS_GEMINILAKE(dev_priv) && IS_REVID(dev_priv, since, until))
+#define IS_GLK_REVID(dev_priv, since, until)		    \
+	(BUILD_BUG_ON_REVID_LT(until, GLK_REVID_PRODUCT) && \
+	 IS_GEMINILAKE(dev_priv) && IS_REVID(dev_priv, since, until))
 
 #define CNL_REVID_A0		0x0
 #define CNL_REVID_B0		0x1
 #define CNL_REVID_C0		0x2
+#define CNL_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
 
 #define IS_CNL_REVID(p, since, until) \
-	(IS_CANNONLAKE(p) && IS_REVID(p, since, until))
+	(BUILD_BUG_ON_REVID_LT(until, CNL_REVID_PRODUCT) && \
+	 IS_CANNONLAKE(p) && IS_REVID(p, since, until))
 
 #define ICL_REVID_A0		0x0
 #define ICL_REVID_A2		0x1
 #define ICL_REVID_B0		0x3
 #define ICL_REVID_B2		0x4
 #define ICL_REVID_C0		0x5
+#define ICL_REVID_PRODUCT	PRODUCT_REVID_UNKNOWN
 
 #define IS_ICL_REVID(p, since, until) \
-	(IS_ICELAKE(p) && IS_REVID(p, since, until))
+	(BUILD_BUG_ON_REVID_LT(until, ICL_REVID_PRODUCT) && \
+	IS_ICELAKE(p) && IS_REVID(p, since, until))
 
 /*
  * The genX designation typically refers to the render engine, so render