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 <[email protected]> > > > > > Cc: Jani Nikula <[email protected]> > > > > > Cc: Imre Deak <[email protected]> > > > > > Fixes: commit ec7e0bb35f8d ("drm/i915/cnp: Add PCI ID for Cannonpoint > > > > > LP PCH") > > > > > Reported-by: Imre Deak <[email protected]> > > > > > Signed-off-by: Dhinakaran Pandiyan <[email protected]> > > > > > --- > > > > > 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 <[email protected]> > > > > > > > > > > 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 > > > > [email protected] > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > > Intel-gfx mailing list > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
