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

Sagar Arun Kamble
2018-02-17T14:17:59Z

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.
>> 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

--
Thanks,
Sagar