[Intel-gfx] [PATCH 1/6] drm/i915: Store first production revid into device info

[Chris Wilson]

Quoting Mika Kuoppala (2018-06-08 09:39:06)
> Store first known production revid into the device info.
> 
> This enables us to easily see if we are running on
> a preproduction hardware.
> 
> Uninitialized (zero) product revision id means that
> there are no known preliminary hardware for this platform,
> or that the platform is of gen that we don't care.
> This is all pre gen9 platforms.
> 
> Unknown product revision maps to REVID_FOREVER on a
> gen9+ platforms on default. When the platform
> gets the first production revision and our testing
> infra is cleaned from preproduction hardware, we can
> set a first production revid. At that point we start
> to complain about running driver on preliminary hardware.
> 
> v2: initialize GEN9_FEATURES too (CI)
> 
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c          |  6 +++---
>  drivers/gpu/drm/i915/i915_drv.h          |  7 +++++++
>  drivers/gpu/drm/i915/i915_pci.c          | 10 ++++++++--
>  drivers/gpu/drm/i915/intel_device_info.h | 11 +++++++++++
>  4 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index be71fdf8d92e..92f244c12f1e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -855,10 +855,10 @@ static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv)
>         bool pre = false;
>  
>         pre |= IS_HSW_EARLY_SDV(dev_priv);
> -       pre |= IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0);
> -       pre |= IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST);
> +       pre |= IS_PREPRODUCTION_HW(dev_priv);
>  
> -       if (pre) {
> +       if (pre && FIRST_PRODUCT_REVID(INTEL_INFO(dev_priv))
> +           != PRODUCT_REVID_UNKNOWN) {
>                 DRM_ERROR("This is a pre-production stepping. "
>                           "It may not be fully functional.\n");
>                 add_taint(TAINT_MACHINE_CHECK, LOCKDEP_STILL_OK);

I know this is changed later, but this is not very pleasing on the eye.

	const struct intel_info *info = INTEL_INFO(dev_priv);

	if (IS_PREPRODUCTION_HW(dev_priv) &&
	    FIRST_PRODUCT_REVID(info) != PRODUCT_REVID_UNKNOWN) {


> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 933e31669557..9ae9dc553192 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -146,6 +146,17 @@ struct intel_device_info {
>         u16 device_id;
>         u16 gen_mask;
>  
> +       u8 first_product_revid;
> +       /* Set to corresponding first production hardware revision or:

/*
 * Set to

That's the style we're meant to use. It helps when we mix in kerneldoc
comments, e.g.
/**
 * Foo:

> +        *
> +        * 0x00 == uninitialized == no known preliminary hw (legacy gens)
> +        * 0xff == PRODUCT_REVID_UNKNOWN == no known production hw yet
> +        *
> +        * Do not set first product revid unless you are certain
> +        * that testing infrastructure is already on top of production
> +        * revid machines.
> +        */

Description block before member or short description on the same line.
-Chris