Re: [PATCH 5/5] drm/i915: Always use 9 bits of the LPC bridge device ID for PCH detection

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 20 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the code less confusiong by always using the top 9 bits of the
> LPC bridge device ID to detect the PCH type. We need to add a bit of
> new code for WPT, and we need to adjust the KBP ID as well. All the
> other pre-CNP IDs are fine as is.

Seems to me this fixes a (theoretical?) bug with KBP matching some
Lewisburg parts (a2xx).

> The CNP ID I guessed based on the KBP + CNP LP IDs. But someone else
> should really verify this.

Sorry, couldn't find definite info on this.

> The virtualization cases I think are fine. These P2X and P3X IDs
> actually just look like the old PIIX4 and PIIX3 IDs to me. Not sure
> why they're not called PIIX3/4 though. The qemu one has a comment
> saying the full ID is 0x2918 which is fine with 9 bits.

Nor this. This starts ignoring some 71xx device ids, but I'm guessing
they're not relevant.

Not as strong confidence as I'd like, but

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


PS. I thought about switching this to array based matching. It could
define the mask for each id to be matched, so you could e.g. add qemu as
0x2918. And you could have the debug strings there too. But I couldn't
think of a neat way to do the platform/pch cross checks (callbacks don't
count as neat). So I guess this is good enough for now.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 35 ++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/i915_drv.h | 15 +++++++++------
>  2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1681be8d21f6..bfb39047f5e1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -173,29 +173,25 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  	while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
>  		if (pch->vendor == PCI_VENDOR_ID_INTEL) {

Someone should switch this to "if (vendor != intel) continue" when the
dust has settled, and free up some horizontal screen real estate.

>  			unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> -			unsigned short id_ext = pch->device &
> -				INTEL_PCH_DEVICE_ID_MASK_EXT;
> +
> +			dev_priv->pch_id = id;
>  
>  			if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_IBX;
>  				DRM_DEBUG_KMS("Found Ibex Peak PCH\n");
>  				WARN_ON(!IS_GEN5(dev_priv));
>  			} else if (id == INTEL_PCH_CPT_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_CPT;
>  				DRM_DEBUG_KMS("Found CougarPoint PCH\n");
>  				WARN_ON(!IS_GEN6(dev_priv) &&
>  					!IS_IVYBRIDGE(dev_priv));
>  			} else if (id == INTEL_PCH_PPT_DEVICE_ID_TYPE) {
>  				/* PantherPoint is CPT compatible */
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_CPT;
>  				DRM_DEBUG_KMS("Found PantherPoint PCH\n");
>  				WARN_ON(!IS_GEN6(dev_priv) &&
>  					!IS_IVYBRIDGE(dev_priv));
>  			} else if (id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_LPT;
>  				DRM_DEBUG_KMS("Found LynxPoint PCH\n");
>  				WARN_ON(!IS_HASWELL(dev_priv) &&
> @@ -203,39 +199,49 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  				WARN_ON(IS_HSW_ULT(dev_priv) ||
>  					IS_BDW_ULT(dev_priv));
>  			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_LPT;
>  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
>  				WARN_ON(!IS_HASWELL(dev_priv) &&
>  					!IS_BROADWELL(dev_priv));
>  				WARN_ON(!IS_HSW_ULT(dev_priv) &&
>  					!IS_BDW_ULT(dev_priv));
> +			} else if (id == INTEL_PCH_WPT_DEVICE_ID_TYPE) {
> +				/* WildcatPoint is LPT compatible */
> +				dev_priv->pch_type = PCH_LPT;
> +				DRM_DEBUG_KMS("Found WildcatPoint PCH\n");
> +				WARN_ON(!IS_HASWELL(dev_priv) &&
> +					!IS_BROADWELL(dev_priv));
> +				WARN_ON(IS_HSW_ULT(dev_priv) ||
> +					IS_BDW_ULT(dev_priv));
> +			} else if (id == INTEL_PCH_WPT_LP_DEVICE_ID_TYPE) {
> +				/* WildcatPoint is LPT compatible */
> +				dev_priv->pch_type = PCH_LPT;
> +				DRM_DEBUG_KMS("Found WildcatPoint LP PCH\n");
> +				WARN_ON(!IS_HASWELL(dev_priv) &&
> +					!IS_BROADWELL(dev_priv));
> +				WARN_ON(!IS_HSW_ULT(dev_priv) &&
> +					!IS_BDW_ULT(dev_priv));
>  			} else if (id == INTEL_PCH_SPT_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_SPT;
>  				DRM_DEBUG_KMS("Found SunrisePoint PCH\n");
>  				WARN_ON(!IS_SKYLAKE(dev_priv) &&
>  					!IS_KABYLAKE(dev_priv));
> -			} else if (id_ext == INTEL_PCH_SPT_LP_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id_ext;
> +			} else if (id == INTEL_PCH_SPT_LP_DEVICE_ID_TYPE) {
>  				dev_priv->pch_type = PCH_SPT;
>  				DRM_DEBUG_KMS("Found SunrisePoint LP PCH\n");
>  				WARN_ON(!IS_SKYLAKE(dev_priv) &&
>  					!IS_KABYLAKE(dev_priv));
>  			} else if (id == INTEL_PCH_KBP_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_KBP;
>  				DRM_DEBUG_KMS("Found KabyPoint PCH\n");
>  				WARN_ON(!IS_SKYLAKE(dev_priv) &&
>  					!IS_KABYLAKE(dev_priv));
>  			} else if (id == INTEL_PCH_CNP_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type = PCH_CNP;
>  				DRM_DEBUG_KMS("Found CannonPoint PCH\n");
>  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
>  					!IS_COFFEELAKE(dev_priv));
> -			} else if (id_ext == INTEL_PCH_CNP_LP_DEVICE_ID_TYPE) {
> -				dev_priv->pch_id = id_ext;
> +			} else if (id == INTEL_PCH_CNP_LP_DEVICE_ID_TYPE) {
>  				dev_priv->pch_type = PCH_CNP;
>  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
>  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> @@ -247,7 +253,6 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  					    PCI_SUBVENDOR_ID_REDHAT_QUMRANET &&
>  				    pch->subsystem_device ==
>  					    PCI_SUBDEVICE_ID_QEMU)) {
> -				dev_priv->pch_id = id;
>  				dev_priv->pch_type =
>  					intel_virt_detect_pch(dev_priv);
>  			} else
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1b97eb098ffe..4a884cf9b47d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2996,17 +2996,18 @@ intel_info(const struct drm_i915_private *dev_priv)
>  
>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
>  
> -#define INTEL_PCH_DEVICE_ID_MASK		0xff00
> -#define INTEL_PCH_DEVICE_ID_MASK_EXT		0xff80
> +#define INTEL_PCH_DEVICE_ID_MASK		0xff80
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
>  #define INTEL_PCH_PPT_DEVICE_ID_TYPE		0x1e00
>  #define INTEL_PCH_LPT_DEVICE_ID_TYPE		0x8c00
>  #define INTEL_PCH_LPT_LP_DEVICE_ID_TYPE		0x9c00
> +#define INTEL_PCH_WPT_DEVICE_ID_TYPE		0x8c80
> +#define INTEL_PCH_WPT_LP_DEVICE_ID_TYPE		0x9c80
>  #define INTEL_PCH_SPT_DEVICE_ID_TYPE		0xA100
>  #define INTEL_PCH_SPT_LP_DEVICE_ID_TYPE		0x9D00
> -#define INTEL_PCH_KBP_DEVICE_ID_TYPE		0xA200
> -#define INTEL_PCH_CNP_DEVICE_ID_TYPE		0xA300
> +#define INTEL_PCH_KBP_DEVICE_ID_TYPE		0xA280
> +#define INTEL_PCH_CNP_DEVICE_ID_TYPE		0xA380
>  #define INTEL_PCH_CNP_LP_DEVICE_ID_TYPE		0x9D80
>  #define INTEL_PCH_P2X_DEVICE_ID_TYPE		0x7100
>  #define INTEL_PCH_P3X_DEVICE_ID_TYPE		0x7000
> @@ -3020,9 +3021,11 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_PCH_SPT(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_SPT)
>  #define HAS_PCH_LPT(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_LPT)
>  #define HAS_PCH_LPT_LP(dev_priv) \
> -	((dev_priv)->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
> +	((dev_priv)->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE || \
> +	 (dev_priv)->pch_id == INTEL_PCH_WPT_LP_DEVICE_ID_TYPE)
>  #define HAS_PCH_LPT_H(dev_priv) \
> -	((dev_priv)->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE)
> +	((dev_priv)->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE || \
> +	 (dev_priv)->pch_id == INTEL_PCH_WPT_DEVICE_ID_TYPE)
>  #define HAS_PCH_CPT(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_CPT)
>  #define HAS_PCH_IBX(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_IBX)
>  #define HAS_PCH_NOP(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_NOP)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx