[Intel-gfx] [DMC_BUGFIX_SKL_V2 1/5] drm/i915/skl: Added a check for the hardware status of csr fw before loading.

[Animesh Manna]

On 9/2/2015 2:24 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 07:40:54PM +0530, Animesh Manna wrote:
>>
>> On 8/26/2015 6:40 PM, Daniel Vetter wrote:
>>> On Wed, Aug 26, 2015 at 01:36:05AM +0530, Animesh Manna wrote:
>>>> Dmc will restore the csr program except DC9, cold boot,
>>>> warm reset, PCI function level reset, and hibernate/suspend.
>>>>
>>>> intel_csr_load_program() function is used to load the firmware
>>>> data from kernel memory to csr address space.
>>>>
>>>> All values of csr address space will be zero if it got reset and
>>>> the first byte of csr program is always a non-zero if firmware
>>>> is loaded successfuly. Based on hardware status will load the
>>>> firmware.
>>>>
>>>> Without this condition check if we overwrite the firmware data the
>>>> counters exposed for dc5/dc6 (help for debugging) will be nullified.
>> Bacause of the above reason mentioned just above we need to block firmware loading again.
>> So only WARN_ON will not help.
>>
>>
>>>> v1: Initial version.
>>>>
>>>> v2: Based on review comments from Daniel,
>>>> - Added a check to know hardware status and load the firmware if not loaded.
>>>>
>>>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>>>> Cc: Damien Lespiau <damien.lespiau at intel.com>
>>>> Cc: Imre Deak <imre.deak at intel.com>
>>>> Cc: Sunil Kamath <sunil.kamath at intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
>>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>>> index ba1ae03..682cc26 100644
>>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>>> @@ -252,6 +252,15 @@ void intel_csr_load_program(struct drm_device *dev)
>>>>   		return;
>>>>   	}
>>>> +	/*
>>>> +	 * Dmc will restore the csr the program except DC9, cold boot,
>>>> +	 * warm reset, PCI function level reset, and hibernate/suspend.
>>>> +	 * This condition will help to check if csr address space is reset/
>>>> +	 * not loaded.
>>>> +	 */
>>> Atm we call this from driver load and resume, which doesn seem to cover
>>> all the cases you mention in the comment. Should this be a WARN_ON
>>> instead? Or do we have troubles in our init sequence where we load too
>>> many times?
>> Yes, the above statement taken from bspec to describe about the special cases dmc will not restore the firmware.
>> Agree, In our cases cold boot and hibernate/suspend mainly we need to load the firmware again, so in my
>> second sentence I wanted to comment mainly regarding this condition check added for suspend-hibernate(reset)
>> and cold boot(not loaded).
>>
>> Anyways the same api later can be used to load the firmware from anywhere, so my intention to check firmware loaded or not.
>> If already loaded then not to overwrite the csr address space to maintain the dc5/dc6 counter value.
>>
>> Can the below comment more clear to you.
>>
>> 	/*
>> 	 * Dmc will restore the csr the program except DC9, cold boot,
>> 	 * warm reset, PCI function level reset, and hibernate/suspend.
>> 	 * If firmware is restored by dmc then no need to load again which
>> 	 * will keep the dc5/dc6 counter exposed by firmware.
>> 	 */
>>
>> No issue in init sequence.
> That seems to still cover all the callers of the function afaics - we do
> pci resets over suspend resume unconditionally. So I still don't
> understand where exactly we try to load the dmc firmware in i915.ko when
> it's already loaded.

During resume intel_csr_load_program() will be called from
intel_runtime_resume().

intel_runtime_resume()-> skl_resume_prepare()-> intel_csr_load_program()

During Pc10 entry testing I can see dmc is restoring back the firmware always,
but as you mentioned pci-reset can happen unconditionally, but still then
also during resume intel_runtime_resume() will be called and based on
register read of csr-base-address firmware loading will happen.

-Animesh

> -Daniel