* [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment
@ 2018-05-28 4:46 colin.xu
2018-05-28 4:46 ` [PATCH 2/2] drm/i915: Assign PCH_NONE for BXT in virtualization colin.xu
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: colin.xu @ 2018-05-28 4:46 UTC (permalink / raw)
To: jani.nikula, intel-gfx
From: Colin Xu <colin.xu@intel.com>
In recent virtual PCH detection refactoring patch, virtual pch id
is guessed and sanity checked from dev_priv. However, on platform that
has no PCH like BXT, the guessing is wrong by mixing up PCH_NONE with
PCH_NOP.
The patch set handles such situation so that correct virtual PCH_NONE or
PCH_NOP type could be assigned accordingly. And it also add BXT to virtual
PCH detection logic.
Colin Xu (2):
drm/i915: Update virtual PCH in single function
drm/i915: Assign PCH_NONE for BXT in virtualization
drivers/gpu/drm/i915/i915_drv.c | 58 +++++++++++++++++++--------------
1 file changed, 33 insertions(+), 25 deletions(-)
--
2.17.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 2/2] drm/i915: Assign PCH_NONE for BXT in virtualization 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu @ 2018-05-28 4:46 ` colin.xu 2018-05-28 4:46 ` [PATCH 1/2] drm/i915: Update virtual PCH in single function colin.xu ` (3 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: colin.xu @ 2018-05-28 4:46 UTC (permalink / raw) To: jani.nikula, intel-gfx From: Colin Xu <colin.xu@intel.com> On BXT platform, guest kernel request PCH_NONE to initialize display correctly. Signed-off-by: Colin Xu <colin.xu@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 637ba86104be..a09516a348e1 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -245,6 +245,10 @@ intel_virt_update_pch(struct drm_i915_private *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 if (IS_BROXTON(dev_priv)) { + id = 0; + pch_type = intel_pch_type(dev_priv, id); + DRM_DEBUG_KMS("Assuming no PCH\n"); } else { id = 0; pch_type = PCH_NOP; -- 2.17.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu 2018-05-28 4:46 ` [PATCH 2/2] drm/i915: Assign PCH_NONE for BXT in virtualization colin.xu @ 2018-05-28 4:46 ` colin.xu 2018-05-28 9:37 ` Jani Nikula 2018-05-28 5:12 ` ✓ Fi.CI.BAT: success for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment Patchwork ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: colin.xu @ 2018-05-28 4:46 UTC (permalink / raw) To: jani.nikula, intel-gfx 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. 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; } } -- 2.17.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-28 4:46 ` [PATCH 1/2] drm/i915: Update virtual PCH in single function colin.xu @ 2018-05-28 9:37 ` Jani Nikula 2018-05-28 9:48 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2018-05-28 9:37 UTC (permalink / raw) To: colin.xu, intel-gfx 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; > } > } -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-28 9:37 ` Jani Nikula @ 2018-05-28 9:48 ` Jani Nikula 2018-05-28 13:42 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2018-05-28 9:48 UTC (permalink / raw) To: colin.xu, intel-gfx 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; >> } >> } -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-28 9:48 ` Jani Nikula @ 2018-05-28 13:42 ` Jani Nikula 2018-05-29 0:31 ` Colin Xu 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2018-05-28 13:42 UTC (permalink / raw) To: colin.xu, intel-gfx On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: > 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. -ENOCOFFEE in the morning. Is the fix you're looking for simply: diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9c449b8d8eab..ae07e36e364c 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) if (WARN_ON(pch_type == PCH_NONE)) pch_type = PCH_NOP; } else { - pch_type = PCH_NOP; + pch_type = PCH_NONE; } dev_priv->pch_type = pch_type; dev_priv->pch_id = id; --- BR, Jani. > > > 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; >>> } >>> } -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-28 13:42 ` Jani Nikula @ 2018-05-29 0:31 ` Colin Xu 2018-05-29 5:45 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Colin Xu @ 2018-05-29 0:31 UTC (permalink / raw) To: Jani Nikula, intel-gfx On 05/28/2018 09:42 PM, Jani Nikula wrote: > On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: >> 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. > -ENOCOFFEE in the morning. Is the fix you're looking for simply: Yes this is the most simply way. The reason I didn't craft the patch like this in the beginning is that I'm not sure after your refactoring patch, if the case exists that pch id 0 maps to type either nop or none. As you said there is no such case, the simply change should work well. Will you made the change sometime or I need update my patch set? -- Best Regards, Colin Xu > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9c449b8d8eab..ae07e36e364c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) > if (WARN_ON(pch_type == PCH_NONE)) > pch_type = PCH_NOP; > } else { > - pch_type = PCH_NOP; > + pch_type = PCH_NONE; > } > dev_priv->pch_type = pch_type; > dev_priv->pch_id = id; > > --- > > BR, > Jani. > > >> >> 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; >>>> } >>>> } _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-29 0:31 ` Colin Xu @ 2018-05-29 5:45 ` Jani Nikula 2018-05-29 6:20 ` Colin Xu 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2018-05-29 5:45 UTC (permalink / raw) To: Colin Xu, intel-gfx On Wed, 30 May 2018, Colin Xu <Colin.Xu@intel.com> wrote: > On 05/28/2018 09:42 PM, Jani Nikula wrote: >> On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: >>> 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. >> -ENOCOFFEE in the morning. Is the fix you're looking for simply: > > Yes this is the most simply way. > The reason I didn't craft the patch like this in the beginning is that > I'm not sure after your refactoring patch, if the case exists that pch > id 0 maps to type either nop or none. > As you said there is no such case, the simply change should work well. > Will you made the change sometime or I need update my patch set? I was trying to look at which part of my refactoring broke this, but it seems to me it was already setting pch_type to PCH_NOP before that. Do you have a bisect result? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-29 5:45 ` Jani Nikula @ 2018-05-29 6:20 ` Colin Xu 2018-05-31 3:10 ` Xu, Colin 0 siblings, 1 reply; 14+ messages in thread From: Colin Xu @ 2018-05-29 6:20 UTC (permalink / raw) To: Jani Nikula, intel-gfx On 05/29/2018 01:45 PM, Jani Nikula wrote: > On Wed, 30 May 2018, Colin Xu <Colin.Xu@intel.com> wrote: >> On 05/28/2018 09:42 PM, Jani Nikula wrote: >>> On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: >>>> 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. >>> -ENOCOFFEE in the morning. Is the fix you're looking for simply: >> Yes this is the most simply way. >> The reason I didn't craft the patch like this in the beginning is that >> I'm not sure after your refactoring patch, if the case exists that pch >> id 0 maps to type either nop or none. >> As you said there is no such case, the simply change should work well. >> Will you made the change sometime or I need update my patch set? > I was trying to look at which part of my refactoring broke this, but it > seems to me it was already setting pch_type to PCH_NOP before that. > > Do you have a bisect result? It doesnt' seem being broken by the refactoring. Since Broxton isn't supported in virtualization environemtn before, the issue is covered up. In native case, intel_pch_type() returns none since there is no PCH hardware and it works correctly. In virutalization, we expect the same. -- Best Regards, Colin Xu > > BR, > Jani. > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-29 6:20 ` Colin Xu @ 2018-05-31 3:10 ` Xu, Colin 2018-05-31 5:59 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Xu, Colin @ 2018-05-31 3:10 UTC (permalink / raw) To: Xu, Colin, Nikula, Jani; +Cc: intel-gfx@lists.freedesktop.org >-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Colin Xu >Sent: Wednesday, May 30, 2018 14:26 >To: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Update virtual PCH in single function > >On 05/29/2018 01:45 PM, Jani Nikula wrote: > >> On Wed, 30 May 2018, Colin Xu <Colin.Xu@intel.com> wrote: >>> On 05/28/2018 09:42 PM, Jani Nikula wrote: >>>> On Mon, 28 May 2018, Jani Nikula <jani.nikula@intel.com> wrote: >>>>> 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. >>>> -ENOCOFFEE in the morning. Is the fix you're looking for simply: >>> Yes this is the most simply way. >>> The reason I didn't craft the patch like this in the beginning is that >>> I'm not sure after your refactoring patch, if the case exists that pch >>> id 0 maps to type either nop or none. >>> As you said there is no such case, the simply change should work well. >>> Will you made the change sometime or I need update my patch set? >> I was trying to look at which part of my refactoring broke this, but it >> seems to me it was already setting pch_type to PCH_NOP before that. >> >> Do you have a bisect result? > >It doesnt' seem being broken by the refactoring. Since Broxton isn't supported >in virtualization environemtn before, the issue is covered up. >In native case, intel_pch_type() returns none since there is no PCH hardware >and it works correctly. In virutalization, we expect the same. >-- Hi Jani, any comments? Without correct PCH type, BXT in virtualization will fail to boot due to display initialization fail. If any more input required, please kindly let me know. BR, Colin Xu > >> >> BR, >> Jani. >> >> >> > >_______________________________________________ >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] 14+ messages in thread
* Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function 2018-05-31 3:10 ` Xu, Colin @ 2018-05-31 5:59 ` Jani Nikula 0 siblings, 0 replies; 14+ messages in thread From: Jani Nikula @ 2018-05-31 5:59 UTC (permalink / raw) To: Xu, Colin; +Cc: intel-gfx@lists.freedesktop.org On Thu, 31 May 2018, "Xu, Colin" <colin.xu@intel.com> wrote: > Hi Jani, any comments? Without correct PCH type, BXT in virtualization > will fail to boot due to display initialization fail. If any more > input required, please kindly let me know. See [1] and please provide your Tested-by and/or Reviewed-by on the relevant patches. Thanks, Jani. [1] https://patchwork.freedesktop.org/series/43986/ -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu 2018-05-28 4:46 ` [PATCH 2/2] drm/i915: Assign PCH_NONE for BXT in virtualization colin.xu 2018-05-28 4:46 ` [PATCH 1/2] drm/i915: Update virtual PCH in single function colin.xu @ 2018-05-28 5:12 ` Patchwork 2018-05-28 6:02 ` ✓ Fi.CI.IGT: " Patchwork 2018-05-28 13:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment (rev2) Patchwork 4 siblings, 0 replies; 14+ messages in thread From: Patchwork @ 2018-05-28 5:12 UTC (permalink / raw) To: colin.xu; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment URL : https://patchwork.freedesktop.org/series/43833/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4247 -> Patchwork_9133 = == Summary - SUCCESS == No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/43833/revisions/1/mbox/ == Known issues == Here are the changes found in Patchwork_9133 that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_module_reload@basic-reload: fi-glk-j4005: PASS -> DMESG-WARN (fdo#106248) igt@gem_exec_suspend@basic-s4-devices: fi-kbl-7500u: PASS -> DMESG-WARN (fdo#105128) igt@kms_flip@basic-flip-vs-wf_vblank: fi-glk-j4005: PASS -> FAIL (fdo#100368) igt@kms_pipe_crc_basic@read-crc-pipe-c: fi-glk-j4005: PASS -> DMESG-WARN (fdo#106097) igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: fi-bxt-dsi: PASS -> INCOMPLETE (fdo#103927) ==== Possible fixes ==== igt@drv_module_reload@basic-reload: fi-ilk-650: DMESG-WARN -> PASS fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128 fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097 fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248 == Participating hosts (43 -> 39) == Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-skl-6700hq == Build changes == * Linux: CI_DRM_4247 -> Patchwork_9133 CI_DRM_4247: c0a490c288fea47c3eb902b2b4e271955cc94fbf @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4499: f560ae5a464331f03f0a669ed46b8c9e56526187 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_9133: b06f576c2614ca347a2180e8842d957657179dfa @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == b06f576c2614 drm/i915: Assign PCH_NONE for BXT in virtualization b71b6cb2fc66 drm/i915: Update virtual PCH in single function == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9133/issues.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu ` (2 preceding siblings ...) 2018-05-28 5:12 ` ✓ Fi.CI.BAT: success for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment Patchwork @ 2018-05-28 6:02 ` Patchwork 2018-05-28 13:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment (rev2) Patchwork 4 siblings, 0 replies; 14+ messages in thread From: Patchwork @ 2018-05-28 6:02 UTC (permalink / raw) To: colin.xu; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment URL : https://patchwork.freedesktop.org/series/43833/ State : success == Summary == = CI Bug Log - changes from CI_DRM_4247_full -> Patchwork_9133_full = == Summary - WARNING == Minor unknown changes coming with Patchwork_9133_full need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_9133_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://patchwork.freedesktop.org/api/1.0/series/43833/revisions/1/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in Patchwork_9133_full: === IGT changes === ==== Warnings ==== igt@gem_exec_schedule@deep-blt: shard-kbl: PASS -> SKIP == Known issues == Here are the changes found in Patchwork_9133_full that come from known issues: === IGT changes === ==== Issues hit ==== igt@drv_selftest@live_hangcheck: shard-kbl: PASS -> DMESG-FAIL (fdo#106560) igt@kms_setmode@basic: shard-apl: PASS -> FAIL (fdo#99912) igt@perf@blocking: shard-hsw: PASS -> FAIL (fdo#102252) ==== Possible fixes ==== igt@drv_selftest@live_gtt: shard-kbl: FAIL -> PASS shard-glk: INCOMPLETE (fdo#103359, k.org#198133) -> PASS igt@gem_softpin@noreloc-s3: shard-snb: DMESG-WARN (fdo#102365) -> PASS igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing: shard-glk: FAIL (fdo#105703) -> PASS igt@kms_cursor_crc@cursor-256x256-suspend: shard-kbl: INCOMPLETE (fdo#103665) -> PASS igt@kms_flip@2x-flip-vs-absolute-wf_vblank: shard-hsw: FAIL -> PASS igt@kms_flip@basic-flip-vs-wf_vblank: shard-hsw: FAIL (fdo#103928) -> PASS igt@kms_flip@plain-flip-fb-recreate: shard-glk: FAIL (fdo#100368) -> PASS +1 igt@kms_setmode@basic: shard-hsw: FAIL (fdo#99912) -> PASS fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365 fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359 fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665 fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928 fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703 fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560 fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133 == Participating hosts (5 -> 5) == No changes in participating hosts == Build changes == * Linux: CI_DRM_4247 -> Patchwork_9133 CI_DRM_4247: c0a490c288fea47c3eb902b2b4e271955cc94fbf @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4499: f560ae5a464331f03f0a669ed46b8c9e56526187 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_9133: b06f576c2614ca347a2180e8842d957657179dfa @ git://anongit.freedesktop.org/gfx-ci/linux == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9133/shards.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment (rev2) 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu ` (3 preceding siblings ...) 2018-05-28 6:02 ` ✓ Fi.CI.IGT: " Patchwork @ 2018-05-28 13:40 ` Patchwork 4 siblings, 0 replies; 14+ messages in thread From: Patchwork @ 2018-05-28 13:40 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment (rev2) URL : https://patchwork.freedesktop.org/series/43833/ State : failure == Summary == Applying: drm/i915: Update virtual PCH in single function Applying: drm/i915: Assign PCH_NONE for BXT in virtualization error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_drv.c). error: could not build fake ancestor Patch failed at 0002 drm/i915: Assign PCH_NONE for BXT in virtualization The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-05-31 5:59 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-28 4:46 [PATCH 0/2] drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment colin.xu 2018-05-28 4:46 ` [PATCH 2/2] drm/i915: Assign PCH_NONE for BXT in virtualization colin.xu 2018-05-28 4:46 ` [PATCH 1/2] drm/i915: Update virtual PCH in single function colin.xu 2018-05-28 9:37 ` Jani Nikula 2018-05-28 9:48 ` Jani Nikula 2018-05-28 13:42 ` Jani Nikula 2018-05-29 0:31 ` Colin Xu 2018-05-29 5:45 ` Jani Nikula 2018-05-29 6:20 ` Colin Xu 2018-05-31 3:10 ` Xu, Colin 2018-05-31 5:59 ` Jani Nikula 2018-05-28 5:12 ` ✓ Fi.CI.BAT: success for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment Patchwork 2018-05-28 6:02 ` ✓ Fi.CI.IGT: " Patchwork 2018-05-28 13:40 ` ✗ Fi.CI.BAT: failure for drm/i915: Fix incorrect virtual PCH_NONE/PCH_NOP assignment (rev2) Patchwork
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.