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

[Colin Xu <Colin.Xu@intel.com>]

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