Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 28 May 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Mon, 28 May 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>> On Tue, 29 May 2018, colin.xu@xxxxxxxxx wrote:
>>> From: Colin Xu <colin.xu@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux