Re: [PATCH 12/20] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances

Daniele Ceraolo Spurio
2018-02-20T19:16:48Z

On 17/02/18 06:17, Sagar Arun Kamble wrote:
>
>
> On 2/17/2018 5:48 PM, Chris Wilson wrote:
>> Quoting Sagar Arun Kamble (2018-02-17 12:10:32)
>>>
>>> On 2/17/2018 2:34 PM, Chris Wilson wrote:
>>>> Quoting Sagar Arun Kamble (2018-02-17 08:51:44)
>>>>> Earlier I had thought of calling ASSIGN_FW_DOMAINS_TABLE,
>>>>> ASSIGN_*_MMIO_VFUNCS before intel_uncore_fw_domains_init
>>>>> and use I915_READ here for reading the fuse. But that approach
>>>>> seems to
>>>>> expose the vfuncs and forcewake table before fw domains
>>>>> are initialized. Although we can get to know invalid access to
>>>>> read/write accessors before fw_domains get initialized, current
>>>>> ordering
>>>>> of fw_domains init followed by fw domain table/read/write vfuncs init
>>>>> seems right. So using fw_domains_get/put as suggested above
>>>>> should be the way.
>>>>>
>>>>> Chris, Tvrtko, Mika, do you agree?
>>>> What's the complication with the fw_domains? Do the additional
>>>> powerwell
>>>> depend on the fused status of the extra engines? Does it matter if the
>>>> fw_domain are prepped if they are never used?
>>> Yes. To discover the available VD/VE engines/power domains, fuse needs
>>> to be read under blitter forcewake as
>>> RC6 will be enabled by BIOS. We do have usage of forcewake in IVB to
>>> discover FORCEWAKE_MT availability in fw_domains_init.
>>> It should not be a problem if they are prepped but never used.
>>>> Imo, I would have placed the fused discovery in
>>>> intel_engines_init_mmio() (where we do the setup and can take
>>>> forcewake).
>>>> Then adding something like intel_uncore_reinit_mmio() (which would just
>>>> prune the uncore->fw_domains) after checking fused status with
>>>> commentary
>>>> doesn't seem that horrible.
>>> Yes. This approach looks good too. But, we might want to optimize the
>>> driver_load to avoid this setup at first place
>>> instead of pruning later.
>> How many cycles does it take to run through all domains and set up the
>> register offsets? No mmio access required right, we are just moving
>> memory around without even hitting locked instructions?
> Yes. Latency is minimal. We can go with your suggestion. Will need to
> maintain separation of engine_cs/device_info/uncore update
> through separate functions.

We do currently call fw_domain_reset on all domains at initialization
time, which could write to a register that doesn't exist. We don't wait
for the ack and I assume the write would just be dropped if the power
well is not there so it shouldn't be an issue, but it might be worth
adding a comment in fw_domain_reset to remind us that we can't start
waiting for the ack in there unless we move the fuse read to an earlier
point.

Daniele

>>> Another related setup that can be avoided/pruned is the fw_range for the
>>> engine fused off as it can improve the fw lookup.
>> Which is a bsearch on register range, I doubt that's going to be
>> substantially impacted by removing a few ranges. Where it matters, we
>> should be looking to precalculate the result anyway.
> Yes. seems not so worth.
>> -Chris
>