[Intel-gfx] [PATCH 1/2] drm/i915: Update virtual PCH in single function
Colin Xu
Colin.Xu at intel.com
Wed May 30 00:36:39 UTC 2018
On 05/28/2018 09:42 PM, Jani Nikula wrote:
> On Mon, 28 May 2018, Jani Nikula <jani.nikula at intel.com> wrote:
>> On Mon, 28 May 2018, Jani Nikula <jani.nikula at intel.com> wrote:
>>> On Tue, 29 May 2018, colin.xu at intel.com wrote:
>>>> From: Colin Xu <colin.xu at 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 at 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;
>>>> }
>>>> }
More information about the Intel-gfx
mailing list