| Message ID | 20180529045038.31397-2-colin.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 29 May 2018, colin.xu@intel.com wrote: > From: Colin Xu <colin.xu@intel.com> > > The existing way to update virtual PCH will return wrong PCH type > in case the host doesn't have PCH: > - intel_virt_detect_pch returns guessed PCH id 0 > - id 0 maps to PCH_NOP. >> should be PCH_NONE. > Since PCH_NONE and PCH_NOP are different types, mixing them up > will break vbt initialization logic. > > In addition, to add new none/nop PCH override for a specific > platform, branching need to be added to intel_virt_detect_pch(), > intel_pch_type() and the caller since none/nop PCH is not always > mapping to the same predefined PCH id. > > This patch merges the virtual PCH update/sanity check logic into > single function intel_virt_update_pch(), which still keeps using > existing intel_pch_type() to do the sanity check, while making it > clean to override virtual PCH id for a specific platform for future > platform enablement. Please keep the assignment out of intel_virt_{detect,update}_pch like. I think the patch here is unnecessarily complicated. BR, Jani. > > Signed-off-by: Colin Xu <colin.xu@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 56 ++++++++++++++++++--------------- > 1 file changed, 30 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index fb39e40c0847..637ba86104be 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -209,10 +209,11 @@ static bool intel_is_virt_pch(unsigned short id, > sdevice == PCI_SUBDEVICE_ID_QEMU)); > } > > -static unsigned short > -intel_virt_detect_pch(const struct drm_i915_private *dev_priv) > +static void > +intel_virt_update_pch(struct drm_i915_private *dev_priv) > { > unsigned short id = 0; > + enum intel_pch pch_type = PCH_NONE; > > /* > * In a virtualized passthrough environment we can be in a > @@ -221,25 +222,37 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv) > * make an educated guess as to which PCH is really there. > */ > > - if (IS_GEN5(dev_priv)) > + if (IS_GEN5(dev_priv)) { > id = INTEL_PCH_IBX_DEVICE_ID_TYPE; > - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) > + pch_type = intel_pch_type(dev_priv, id); > + DRM_DEBUG_KMS("Assuming Ibex Peak PCH id %04x\n", id); > + } else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) { > id = INTEL_PCH_CPT_DEVICE_ID_TYPE; > - else if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) > - id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; > - else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > - id = INTEL_PCH_LPT_DEVICE_ID_TYPE; > - else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > + pch_type = intel_pch_type(dev_priv, id); > + DRM_DEBUG_KMS("Assuming CougarPoint PCH id %04x\n", id); > + } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > + if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) > + id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; > + else > + id = INTEL_PCH_LPT_DEVICE_ID_TYPE; > + pch_type = intel_pch_type(dev_priv, id); > + DRM_DEBUG_KMS("Assuming LynxPoint PCH id %04x\n", id); > + } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { > id = INTEL_PCH_SPT_DEVICE_ID_TYPE; > - else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) > + pch_type = intel_pch_type(dev_priv, id); > + DRM_DEBUG_KMS("Assuming SunrisePoint PCH id %04x\n", id); > + } else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { > id = INTEL_PCH_CNP_DEVICE_ID_TYPE; > + pch_type = intel_pch_type(dev_priv, id); > + DRM_DEBUG_KMS("Assuming CannonPoint PCH id %04x\n", id); > + } else { > + id = 0; > + pch_type = PCH_NOP; > + DRM_DEBUG_KMS("Assuming NOP PCH\n"); > + } > > - if (id) > - DRM_DEBUG_KMS("Assuming PCH ID %04x\n", id); > - else > - DRM_DEBUG_KMS("Assuming no PCH\n"); > - > - return id; > + dev_priv->pch_type = pch_type; > + dev_priv->pch_id = id; > } > > static void intel_detect_pch(struct drm_i915_private *dev_priv) > @@ -281,16 +294,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) > break; > } else if (intel_is_virt_pch(id, pch->subsystem_vendor, > pch->subsystem_device)) { > - id = intel_virt_detect_pch(dev_priv); > - if (id) { > - pch_type = intel_pch_type(dev_priv, id); > - if (WARN_ON(pch_type == PCH_NONE)) > - pch_type = PCH_NOP; > - } else { > - pch_type = PCH_NOP; > - } > - dev_priv->pch_type = pch_type; > - dev_priv->pch_id = id; > + intel_virt_update_pch(dev_priv); > break; > } > }
On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > On Tue, 29 May 2018, colin.xu@intel.com wrote: >> From: Colin Xu <colin.xu@intel.com> >> >> The existing way to update virtual PCH will return wrong PCH type >> in case the host doesn't have PCH: >> - intel_virt_detect_pch returns guessed PCH id 0 >> - id 0 maps to PCH_NOP. >> should be PCH_NONE. >> Since PCH_NONE and PCH_NOP are different types, mixing them up >> will break vbt initialization logic. >> >> In addition, to add new none/nop PCH override for a specific >> platform, branching need to be added to intel_virt_detect_pch(), >> intel_pch_type() and the caller since none/nop PCH is not always >> mapping to the same predefined PCH id. >> >> This patch merges the virtual PCH update/sanity check logic into >> single function intel_virt_update_pch(), which still keeps using >> existing intel_pch_type() to do the sanity check, while making it >> clean to override virtual PCH id for a specific platform for future >> platform enablement. > > Please keep the assignment out of intel_virt_{detect,update}_pch like. I > think the patch here is unnecessarily complicated. To elaborate, intel_pch_type() should *always* be able to map pch id to pch type. There should not be combinations that aren't covered by that. If the sanity checks there need to accept Broxton as well, perhaps pass a parameter to indicate virtualization, and accept certain pch ids for Broxton as well. If you're faking a pch for Broxton, I don't think there's a case where pch id should be 0 and pch type should be something else. Either both are zero, or both are non-zero. BR, Jani > > BR, > Jani. > > >> >> Signed-off-by: Colin Xu <colin.xu@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 56 ++++++++++++++++++--------------- >> 1 file changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index fb39e40c0847..637ba86104be 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -209,10 +209,11 @@ static bool intel_is_virt_pch(unsigned short id, >> sdevice == PCI_SUBDEVICE_ID_QEMU)); >> } >> >> -static unsigned short >> -intel_virt_detect_pch(const struct drm_i915_private *dev_priv) >> +static void >> +intel_virt_update_pch(struct drm_i915_private *dev_priv) >> { >> unsigned short id = 0; >> + enum intel_pch pch_type = PCH_NONE; >> >> /* >> * In a virtualized passthrough environment we can be in a >> @@ -221,25 +222,37 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv) >> * make an educated guess as to which PCH is really there. >> */ >> >> - if (IS_GEN5(dev_priv)) >> + if (IS_GEN5(dev_priv)) { >> id = INTEL_PCH_IBX_DEVICE_ID_TYPE; >> - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) >> + pch_type = intel_pch_type(dev_priv, id); >> + DRM_DEBUG_KMS("Assuming Ibex Peak PCH id %04x\n", id); >> + } else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) { >> id = INTEL_PCH_CPT_DEVICE_ID_TYPE; >> - else if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) >> - id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; >> - else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) >> - id = INTEL_PCH_LPT_DEVICE_ID_TYPE; >> - else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) >> + pch_type = intel_pch_type(dev_priv, id); >> + DRM_DEBUG_KMS("Assuming CougarPoint PCH id %04x\n", id); >> + } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { >> + if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) >> + id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; >> + else >> + id = INTEL_PCH_LPT_DEVICE_ID_TYPE; >> + pch_type = intel_pch_type(dev_priv, id); >> + DRM_DEBUG_KMS("Assuming LynxPoint PCH id %04x\n", id); >> + } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { >> id = INTEL_PCH_SPT_DEVICE_ID_TYPE; >> - else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) >> + pch_type = intel_pch_type(dev_priv, id); >> + DRM_DEBUG_KMS("Assuming SunrisePoint PCH id %04x\n", id); >> + } else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { >> id = INTEL_PCH_CNP_DEVICE_ID_TYPE; >> + pch_type = intel_pch_type(dev_priv, id); >> + DRM_DEBUG_KMS("Assuming CannonPoint PCH id %04x\n", id); >> + } else { >> + id = 0; >> + pch_type = PCH_NOP; >> + DRM_DEBUG_KMS("Assuming NOP PCH\n"); >> + } >> >> - if (id) >> - DRM_DEBUG_KMS("Assuming PCH ID %04x\n", id); >> - else >> - DRM_DEBUG_KMS("Assuming no PCH\n"); >> - >> - return id; >> + dev_priv->pch_type = pch_type; >> + dev_priv->pch_id = id; >> } >> >> static void intel_detect_pch(struct drm_i915_private *dev_priv) >> @@ -281,16 +294,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) >> break; >> } else if (intel_is_virt_pch(id, pch->subsystem_vendor, >> pch->subsystem_device)) { >> - id = intel_virt_detect_pch(dev_priv); >> - if (id) { >> - pch_type = intel_pch_type(dev_priv, id); >> - if (WARN_ON(pch_type == PCH_NONE)) >> - pch_type = PCH_NOP; >> - } else { >> - pch_type = PCH_NOP; >> - } >> - dev_priv->pch_type = pch_type; >> - dev_priv->pch_id = id; >> + intel_virt_update_pch(dev_priv); >> break; >> } >> }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index fb39e40c0847..637ba86104be 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -209,10 +209,11 @@ static bool intel_is_virt_pch(unsigned short id, sdevice == PCI_SUBDEVICE_ID_QEMU)); } -static unsigned short -intel_virt_detect_pch(const struct drm_i915_private *dev_priv) +static void +intel_virt_update_pch(struct drm_i915_private *dev_priv) { unsigned short id = 0; + enum intel_pch pch_type = PCH_NONE; /* * In a virtualized passthrough environment we can be in a @@ -221,25 +222,37 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv) * make an educated guess as to which PCH is really there. */ - if (IS_GEN5(dev_priv)) + if (IS_GEN5(dev_priv)) { id = INTEL_PCH_IBX_DEVICE_ID_TYPE; - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming Ibex Peak PCH id %04x\n", id); + } else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) { id = INTEL_PCH_CPT_DEVICE_ID_TYPE; - else if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) - id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; - else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) - id = INTEL_PCH_LPT_DEVICE_ID_TYPE; - else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming CougarPoint PCH id %04x\n", id); + } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { + if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) + id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; + else + id = INTEL_PCH_LPT_DEVICE_ID_TYPE; + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming LynxPoint PCH id %04x\n", id); + } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { id = INTEL_PCH_SPT_DEVICE_ID_TYPE; - else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming SunrisePoint PCH id %04x\n", id); + } else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { id = INTEL_PCH_CNP_DEVICE_ID_TYPE; + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming CannonPoint PCH id %04x\n", id); + } else { + id = 0; + pch_type = PCH_NOP; + DRM_DEBUG_KMS("Assuming NOP PCH\n"); + } - if (id) - DRM_DEBUG_KMS("Assuming PCH ID %04x\n", id); - else - DRM_DEBUG_KMS("Assuming no PCH\n"); - - return id; + dev_priv->pch_type = pch_type; + dev_priv->pch_id = id; } static void intel_detect_pch(struct drm_i915_private *dev_priv) @@ -281,16 +294,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) break; } else if (intel_is_virt_pch(id, pch->subsystem_vendor, pch->subsystem_device)) { - id = intel_virt_detect_pch(dev_priv); - if (id) { - pch_type = intel_pch_type(dev_priv, id); - if (WARN_ON(pch_type == PCH_NONE)) - pch_type = PCH_NOP; - } else { - pch_type = PCH_NOP; - } - dev_priv->pch_type = pch_type; - dev_priv->pch_id = id; + intel_virt_update_pch(dev_priv); break; } }