[Intel-gfx] [PATCH 1/2] drm/i915: Update virtual PCH in single function

[Xu, Colin]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Colin Xu
>Sent: Wednesday, May 30, 2018 14:26
>To: Nikula, Jani <jani.nikula at intel.com>; intel-gfx at 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 at intel.com> wrote:
>>> 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?
>> 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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx