| Message ID | 1497560207-2717-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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 >
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
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
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
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
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
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(-)