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