All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
@ 2017-06-15 20:56 Dhinakaran Pandiyan
  2017-06-15 21:25 ` ✗ Fi.CI.BAT: warning for " Patchwork
  2017-06-16 13:26 ` [PATCH] " Imre Deak
  0 siblings, 2 replies; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2017-06-15 20:56 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, Dhinakaran Pandiyan, Rodrigo Vivi

Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
platforms with LP PCH.

Also, use the extended 9 bits for detecting PCH_LPT_LP.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1f802de..29caf05 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
 			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) &&
 					!IS_BROADWELL(dev_priv));
 				WARN_ON(IS_HSW_ULT(dev_priv) ||
 					IS_BDW_ULT(dev_priv));
-			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+				dev_priv->pch_id = id_ext;
 				dev_priv->pch_type = PCH_LPT;
 				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
 				WARN_ON(!IS_HASWELL(dev_priv) &&
@@ -208,26 +211,31 @@ 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_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;
 				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;
 				dev_priv->pch_type = PCH_CNP;
 				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
 				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
@@ -239,6 +247,7 @@ 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
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* ✗ Fi.CI.BAT: warning for drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-15 20:56 [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH Dhinakaran Pandiyan
@ 2017-06-15 21:25 ` Patchwork
  2017-06-16 13:26 ` [PATCH] " Imre Deak
  1 sibling, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-06-15 21:25 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
URL   : https://patchwork.freedesktop.org/series/25874/
State : warning

== Summary ==

Series 25874v1 drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
https://patchwork.freedesktop.org/api/1.0/series/25874/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215 +1
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup bad-nb-words-3:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup bad-pipe:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup bad-source:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup hang-read-crc-pipe-a:
                fail       -> DMESG-FAIL (fi-skl-6700hq) fdo#101154 +15
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-bdw-5557u)
        Subgroup basic-reload-final:
                pass       -> DMESG-WARN (fi-bdw-5557u)

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101154 https://bugs.freedesktop.org/show_bug.cgi?id=101154

fi-bdw-5557u     total:278  pass:256  dwarn:11  dfail:0   fail:0   skip:11  time:457s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:489s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:588s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:556s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:500s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:590s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:437s
fi-hsw-4770r     total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:16  time:420s
fi-ilk-650       total:278  pass:227  dwarn:0   dfail:0   fail:0   skip:50  time:468s
fi-ivb-3520m     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:18  time:497s
fi-ivb-3770      total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:18  time:518s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:581s
fi-kbl-r         total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time:578s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:483s
fi-skl-6700hq    total:278  pass:224  dwarn:5   dfail:16  fail:11  skip:22  time:487s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:515s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:517s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:515s
fi-snb-2520m     total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:28  time:668s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:0   skip:29  time:412s

3b204b9a85e7f2868671949da81e2d52a5eef76b drm-tip: 2017y-06m-15d-20h-40m-07s UTC integration manifest
a25633c drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH

== Logs ==

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-15 20:56 [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH Dhinakaran Pandiyan
  2017-06-15 21:25 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2017-06-16 13:26 ` Imre Deak
  2017-06-16 19:19   ` Pandiyan, Dhinakaran
  1 sibling, 1 reply; 9+ messages in thread
From: Imre Deak @ 2017-06-16 13:26 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: Jani Nikula, intel-gfx, Rodrigo Vivi

On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> platforms with LP PCH.
> 
> Also, use the extended 9 bits for detecting PCH_LPT_LP.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> Reported-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1f802de..29caf05 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  			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) &&
>  					!IS_BROADWELL(dev_priv));
>  				WARN_ON(IS_HSW_ULT(dev_priv) ||
>  					IS_BDW_ULT(dev_priv));
> -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> +				dev_priv->pch_id = id_ext;

Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
for these Intel PCH devices? It looks like all the existing ones use
less than 8 bits for some kind of HW patch level, but would be good to
have some documentation about this. Anyway if this is need it should be
a separate patch.

Without this part the rest looks ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  				dev_priv->pch_type = PCH_LPT;
>  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
>  				WARN_ON(!IS_HASWELL(dev_priv) &&
> @@ -208,26 +211,31 @@ 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_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;
>  				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;
>  				dev_priv->pch_type = PCH_CNP;
>  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
>  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> @@ -239,6 +247,7 @@ 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
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 13:26 ` [PATCH] " Imre Deak
@ 2017-06-16 19:19   ` Pandiyan, Dhinakaran
  2017-06-16 19:58     ` Imre Deak
  0 siblings, 1 reply; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-16 19:19 UTC (permalink / raw)
  To: Deak, Imre; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Vivi, Rodrigo

On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > platforms with LP PCH.
> > 
> > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > Reported-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1f802de..29caf05 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> >  			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) &&
> >  					!IS_BROADWELL(dev_priv));
> >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> >  					IS_BDW_ULT(dev_priv));
> > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > +				dev_priv->pch_id = id_ext;
> 
> Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> for these Intel PCH devices? It looks like all the existing ones use
> less than 8 bits for some kind of HW patch level, but would be good to
> have some documentation about this. Anyway if this is need it should be
> a separate patch.

I found a document that says 9 bits of Dev ID are needed for
identification. Also, I see references for "Wildcat Point-LP" pch
devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
0x9c0). 


> 
> Without this part the rest looks ok to me:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 

Thanks for the review, I'll drop the change since I am not sure how
useful it is.


> >  				dev_priv->pch_type = PCH_LPT;
> >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > @@ -208,26 +211,31 @@ 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_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;
> >  				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;
> >  				dev_priv->pch_type = PCH_CNP;
> >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > @@ -239,6 +247,7 @@ 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
> > -- 
> > 2.7.4
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 19:19   ` Pandiyan, Dhinakaran
@ 2017-06-16 19:58     ` Imre Deak
  2017-06-16 20:18       ` Ville Syrjälä
  0 siblings, 1 reply; 9+ messages in thread
From: Imre Deak @ 2017-06-16 19:58 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Vivi, Rodrigo

On Fri, Jun 16, 2017 at 10:19:34PM +0300, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> > On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > > platforms with LP PCH.
> > > 
> > > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > > Reported-by: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 1f802de..29caf05 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > >  			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) &&
> > >  					!IS_BROADWELL(dev_priv));
> > >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> > >  					IS_BDW_ULT(dev_priv));
> > > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > +				dev_priv->pch_id = id_ext;
> > 
> > Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> > for these Intel PCH devices? It looks like all the existing ones use
> > less than 8 bits for some kind of HW patch level, but would be good to
> > have some documentation about this. Anyway if this is need it should be
> > a separate patch.
> 
> I found a document that says 9 bits of Dev ID are needed for
> identification. Also, I see references for "Wildcat Point-LP" pch
> devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
> 0x9c0).

Ok, if that doc applies to all PCHs we use I'd suggest a follow-up patch
to this one that changes all of them to 9 bits and adds a reference to the
document.

> > 
> > Without this part the rest looks ok to me:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> 
> Thanks for the review, I'll drop the change since I am not sure how
> useful it is.
> 
> 
> > >  				dev_priv->pch_type = PCH_LPT;
> > >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > > @@ -208,26 +211,31 @@ 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_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;
> > >  				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;
> > >  				dev_priv->pch_type = PCH_CNP;
> > >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> > >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > > @@ -239,6 +247,7 @@ 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
> > > -- 
> > > 2.7.4
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 19:58     ` Imre Deak
@ 2017-06-16 20:18       ` Ville Syrjälä
  2017-06-16 20:35         ` Imre Deak
  0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2017-06-16 20:18 UTC (permalink / raw)
  To: Imre Deak
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org,
	Pandiyan, Dhinakaran, Vivi, Rodrigo

On Fri, Jun 16, 2017 at 10:58:57PM +0300, Imre Deak wrote:
> On Fri, Jun 16, 2017 at 10:19:34PM +0300, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> > > On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > > > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > > > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > > > platforms with LP PCH.
> > > > 
> > > > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > > > Reported-by: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 1f802de..29caf05 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > > >  			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) &&
> > > >  					!IS_BROADWELL(dev_priv));
> > > >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> > > >  					IS_BDW_ULT(dev_priv));
> > > > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > +				dev_priv->pch_id = id_ext;
> > > 
> > > Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> > > for these Intel PCH devices? It looks like all the existing ones use
> > > less than 8 bits for some kind of HW patch level, but would be good to
> > > have some documentation about this. Anyway if this is need it should be
> > > a separate patch.
> > 
> > I found a document that says 9 bits of Dev ID are needed for
> > identification. Also, I see references for "Wildcat Point-LP" pch
> > devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
> > 0x9c0).
> 
> Ok, if that doc applies to all PCHs we use I'd suggest a follow-up patch
> to this one that changes all of them to 9 bits and adds a reference to the
> document.

I think 9 bits would require that we deal with WPT explicitly.

> 
> > > 
> > > Without this part the rest looks ok to me:
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > 
> > 
> > Thanks for the review, I'll drop the change since I am not sure how
> > useful it is.
> > 
> > 
> > > >  				dev_priv->pch_type = PCH_LPT;
> > > >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > > >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > > > @@ -208,26 +211,31 @@ 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_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;
> > > >  				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;
> > > >  				dev_priv->pch_type = PCH_CNP;
> > > >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> > > >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > > > @@ -239,6 +247,7 @@ 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
> > > > -- 
> > > > 2.7.4
> > > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 20:18       ` Ville Syrjälä
@ 2017-06-16 20:35         ` Imre Deak
  2017-06-16 20:41           ` Vivi, Rodrigo
  2017-06-16 20:45           ` Pandiyan, Dhinakaran
  0 siblings, 2 replies; 9+ messages in thread
From: Imre Deak @ 2017-06-16 20:35 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org,
	Pandiyan, Dhinakaran, Vivi, Rodrigo

On Fri, Jun 16, 2017 at 11:18:40PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 16, 2017 at 10:58:57PM +0300, Imre Deak wrote:
> > On Fri, Jun 16, 2017 at 10:19:34PM +0300, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> > > > On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > > > > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > > > > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > > > > platforms with LP PCH.
> > > > > 
> > > > > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > > > > Reported-by: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index 1f802de..29caf05 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > > > >  			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) &&
> > > > >  					!IS_BROADWELL(dev_priv));
> > > > >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> > > > >  					IS_BDW_ULT(dev_priv));
> > > > > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > +				dev_priv->pch_id = id_ext;
> > > > 
> > > > Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> > > > for these Intel PCH devices? It looks like all the existing ones use
> > > > less than 8 bits for some kind of HW patch level, but would be good to
> > > > have some documentation about this. Anyway if this is need it should be
> > > > a separate patch.
> > > 
> > > I found a document that says 9 bits of Dev ID are needed for
> > > identification. Also, I see references for "Wildcat Point-LP" pch
> > > devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
> > > 0x9c0).
> > 
> > Ok, if that doc applies to all PCHs we use I'd suggest a follow-up patch
> > to this one that changes all of them to 9 bits and adds a reference to the
> > document.
> 
> I think 9 bits would require that we deal with WPT explicitly.

Right, so the above change would have actually caused not detecting WPT,
didn't notice that. Imo listing each IDs explicitly and using 9 bits
everywhere would be clearer than the current code.

> 
> > 
> > > > 
> > > > Without this part the rest looks ok to me:
> > > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > 
> > > Thanks for the review, I'll drop the change since I am not sure how
> > > useful it is.
> > > 
> > > 
> > > > >  				dev_priv->pch_type = PCH_LPT;
> > > > >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > > > >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > > > > @@ -208,26 +211,31 @@ 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_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;
> > > > >  				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;
> > > > >  				dev_priv->pch_type = PCH_CNP;
> > > > >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> > > > >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > > > > @@ -239,6 +247,7 @@ 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
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 20:35         ` Imre Deak
@ 2017-06-16 20:41           ` Vivi, Rodrigo
  2017-06-16 20:45           ` Pandiyan, Dhinakaran
  1 sibling, 0 replies; 9+ messages in thread
From: Vivi, Rodrigo @ 2017-06-16 20:41 UTC (permalink / raw)
  To: Deak, Imre
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org,
	Pandiyan, Dhinakaran

On Fri, 2017-06-16 at 23:35 +0300, Imre Deak wrote:
> On Fri, Jun 16, 2017 at 11:18:40PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 16, 2017 at 10:58:57PM +0300, Imre Deak wrote:
> > > On Fri, Jun 16, 2017 at 10:19:34PM +0300, Pandiyan, Dhinakaran wrote:
> > > > On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> > > > > On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > > > > > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > > > > > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > > > > > platforms with LP PCH.
> > > > > > 
> > > > > > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > > > > > Reported-by: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> > > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 1f802de..29caf05 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > > > > >  			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) &&
> > > > > >  					!IS_BROADWELL(dev_priv));
> > > > > >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> > > > > >  					IS_BDW_ULT(dev_priv));
> > > > > > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > > +				dev_priv->pch_id = id_ext;
> > > > > 
> > > > > Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> > > > > for these Intel PCH devices? It looks like all the existing ones use
> > > > > less than 8 bits for some kind of HW patch level, but would be good to
> > > > > have some documentation about this. Anyway if this is need it should be
> > > > > a separate patch.
> > > > 
> > > > I found a document that says 9 bits of Dev ID are needed for
> > > > identification. Also, I see references for "Wildcat Point-LP" pch
> > > > devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
> > > > 0x9c0).
> > > 
> > > Ok, if that doc applies to all PCHs we use I'd suggest a follow-up patch
> > > to this one that changes all of them to 9 bits and adds a reference to the
> > > document.
> > 
> > I think 9 bits would require that we deal with WPT explicitly.
> 
> Right, so the above change would have actually caused not detecting WPT,
> didn't notice that. Imo listing each IDs explicitly and using 9 bits
> everywhere would be clearer than the current code.

I agree, but I'm afraid this won't be an easy task to go behind
searching all available ids :(
The risk of missing something and broking things would be huge...

> 
> > 
> > > 
> > > > > 
> > > > > Without this part the rest looks ok to me:
> > > > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > > > 
> > > > 
> > > > Thanks for the review, I'll drop the change since I am not sure how
> > > > useful it is.
> > > > 
> > > > 
> > > > > >  				dev_priv->pch_type = PCH_LPT;
> > > > > >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > > > > >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > > > > > @@ -208,26 +211,31 @@ 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_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;
> > > > > >  				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;
> > > > > >  				dev_priv->pch_type = PCH_CNP;
> > > > > >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> > > > > >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > > > > > @@ -239,6 +247,7 @@ 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
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH
  2017-06-16 20:35         ` Imre Deak
  2017-06-16 20:41           ` Vivi, Rodrigo
@ 2017-06-16 20:45           ` Pandiyan, Dhinakaran
  1 sibling, 0 replies; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-16 20:45 UTC (permalink / raw)
  To: Deak, Imre; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Vivi, Rodrigo

On Fri, 2017-06-16 at 23:35 +0300, Imre Deak wrote:
> On Fri, Jun 16, 2017 at 11:18:40PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 16, 2017 at 10:58:57PM +0300, Imre Deak wrote:
> > > On Fri, Jun 16, 2017 at 10:19:34PM +0300, Pandiyan, Dhinakaran wrote:
> > > > On Fri, 2017-06-16 at 16:26 +0300, Imre Deak wrote:
> > > > > On Thu, Jun 15, 2017 at 01:56:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > > Although we use 9 bits of Device ID for identifying PCH, only 8 bits are
> > > > > > stored in dev_priv->pch_id. This makes HAS_PCH_CNP_LP() and
> > > > > > HAS_PCH_SPT_LP() incorrect. Fix this by storing all the 9 bits for the
> > > > > > platforms with LP PCH.
> > > > > > 
> > > > > > Also, use the extended 9 bits for detecting PCH_LPT_LP.
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint LP PCH")
> > > > > > Reported-by: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.c | 15 ++++++++++++---
> > > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 1f802de..29caf05 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -176,31 +176,34 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > > > > >  			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) &&
> > > > > >  					!IS_BROADWELL(dev_priv));
> > > > > >  				WARN_ON(IS_HSW_ULT(dev_priv) ||
> > > > > >  					IS_BDW_ULT(dev_priv));
> > > > > > -			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > > +			} else if (id_ext == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> > > > > > +				dev_priv->pch_id = id_ext;
> > > > > 
> > > > > Hm, why to change it? Is there any info about the PCI_DEVICE_ID format
> > > > > for these Intel PCH devices? It looks like all the existing ones use
> > > > > less than 8 bits for some kind of HW patch level, but would be good to
> > > > > have some documentation about this. Anyway if this is need it should be
> > > > > a separate patch.
> > > > 
> > > > I found a document that says 9 bits of Dev ID are needed for
> > > > identification. Also, I see references for "Wildcat Point-LP" pch
> > > > devices starting with a Dev ID 0x9c8 elsewhere in the kernel (LPT-LP is
> > > > 0x9c0).
> > > 
> > > Ok, if that doc applies to all PCHs we use I'd suggest a follow-up patch
> > > to this one that changes all of them to 9 bits and adds a reference to the
> > > document.
> > 
> > I think 9 bits would require that we deal with WPT explicitly.
> 
> Right, so the above change would have actually caused not detecting WPT,
> didn't notice that. Imo listing each IDs explicitly and using 9 bits
> everywhere would be clearer than the current code.
> 

Yeah, did not realize there was a WPT until I grepped the kernel to
check other drivers. Since we do not have any WPT specific code, we
could leave things as they are for older platforms and use 9-bits for
new ones.


> > 
> > > 
> > > > > 
> > > > > Without this part the rest looks ok to me:
> > > > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > > > 
> > > > 
> > > > Thanks for the review, I'll drop the change since I am not sure how
> > > > useful it is.
> > > > 
> > > > 
> > > > > >  				dev_priv->pch_type = PCH_LPT;
> > > > > >  				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
> > > > > >  				WARN_ON(!IS_HASWELL(dev_priv) &&
> > > > > > @@ -208,26 +211,31 @@ 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_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;
> > > > > >  				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;
> > > > > >  				dev_priv->pch_type = PCH_CNP;
> > > > > >  				DRM_DEBUG_KMS("Found CannonPoint LP PCH\n");
> > > > > >  				WARN_ON(!IS_CANNONLAKE(dev_priv) &&
> > > > > > @@ -239,6 +247,7 @@ 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
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-06-16 20:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-15 20:56 [PATCH] drm/i915: Store 9 bits of PCI Device ID for platforms with a LP PCH Dhinakaran Pandiyan
2017-06-15 21:25 ` ✗ Fi.CI.BAT: warning for " Patchwork
2017-06-16 13:26 ` [PATCH] " Imre Deak
2017-06-16 19:19   ` Pandiyan, Dhinakaran
2017-06-16 19:58     ` Imre Deak
2017-06-16 20:18       ` Ville Syrjälä
2017-06-16 20:35         ` Imre Deak
2017-06-16 20:41           ` Vivi, Rodrigo
2017-06-16 20:45           ` Pandiyan, Dhinakaran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.