[Intel-gfx] [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/14/2017 03:43 AM, Joonas Lahtinen wrote:
> On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote:
>> On 12/13/2017 01:34 PM, Michal Wajdeczko wrote:
>>> On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li at intel.com>
>>> wrote:
>>>
>>>> On 12/13/2017 01:11 AM, Joonas Lahtinen wrote:
>>>>> On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:
>>>>>> Hardware may have specific restrictions on GuC WOPCM partition
>>>>>> size versus HuC firmware size. With static WOPCM partitioning,
>>>>>> there's no way to adjust the GuC WOPCM partition size based on
>>>>>> the actual HuC firmware size, so that GuC/HuC loading failure
>>>>>> would occur even if there was enough WOPCM space for both
>>>>>> GuC and HuC firmware.
>>>>> WOPCM being a shared feature of the hardware, it should not go under
>>>>> intel_guc_ prefix.
>>>>>
>>>>> There should be a clear division of what is specific to GuC feature
>>>>> only and what is just a feature that happens to be used by GuC (and
>>>>> equally can be used by HuC too).
>>>> the intel_guc_wopcm here only refers to the wopcm used by
>>>> GuC, this structure only defines the GuC related wopcm info.
>>>> (wopcm partition for GuC). We only need to set these values
>>>> (defined in this structure) to GuC registers. And this structure
>>>> should never be touched if GuC was disabled. so it should be
>>>> a part of GuC.
>>>>
>>> But note that yours intel_guc_wopcm is just one of many wopcm partitions.
>>> I think it would be a good idea to create "intel_wopcm.c|h" and keep
>>> all related code and data there (including verification of early setup
>>> done by bios, wopcpm reporting, partitioning).
>>>
>>> Then we can do rest of the programming right there or just take values
>>> that
>>> will be programmed individually by interested components (but former is
>>> preferred to avoid spreading single feature code over too many places)
>>>
>> The KMD only needs to take care of the setup of the GuC WOPCM partition.
>> Other
>> HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus,
>> the GuC WOPM
>> partitioning is needed only when GuC is enabled and uc firmwares are
>> loaded correctly.
>> The only reason for us to have an intel_wopcm is to maintain the overall
>> WOPCM info
>> such as WOPCM size and base. However, it's not necessary since we can
>> reuse existing
>> driver code to get these info.
> I'd go with Michal here, the WOPCM is its own entity in existence.
> Partitioning defintely sounds like it should be intel_wopcm stuff,
> which may yield intel_wopcm_partition under "guc", so then you are
> still able to reference "guc->wopcm.base" where it makes sense.
>
> And how that partition is programmed to GuC registers for it to be
> used, is then stuff to go under intel_guc. And then you have another
> intel_wopcm_partition for "huc".
>
> We should avoid incorrect abstractions, just to avoid a few lines of
> code. That's how the hardware features seem to exist, that's how we
> should map them in the code.
Thanks for your comments. but I have some different opinions.

Agreed that wopcm exists no matter GuC is enabled or not. And we
can reuse existing code to get/verify related info we need for driver level
description of wopcm. that one reason I don't think we need intel_wopcm.

Regarding the partitioning - We need it only when GuC was enabled. In this
case, it makes sense to do it at least in uc level. Plus, from HW point 
of view,
HW only relies on GuC wopcm offset and size to determine the layout
(or say partitions) of the wopcm. In this case, a good abstraction of 
the HW
interface would be:
   struct guc_wopcm {
     u32 offset;
     u32 size;
   };
   guc_wopcm_setup() - which does actual HW status check and GuC wopcm 
setup.
   guc_wopcm_init() - which init/verify the offset and size values 
required by HW.
That's the second reason I think use of intel_guc_wopcm.c is more accurate
since it reflected the actual HW interface and could be enabled/disabled
along with GuC code.

Regarding the generic abstraction of intel_wopcm_partition for both GuC 
& HuC.
I am not sure what's the benefit of such an abstraction. For two reasons:
a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any 
interface
     to configure the partition for HuC, which means we even won't use 
these info in
     the rest of the driver code.
b) For debugging and tracking propose, we can easily get overall layout 
of WOPCM
     by just using overall wopcm description and GuC wopcm usage.

Please do let me know if anything was wrong :-)

Regards,
-Jackie
> Regards, Joonas