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

[Yaodong Li]

On 12/15/2017 02:21 AM, Joonas Lahtinen wrote:
> On Thu, 2017-12-14 at 20:55 -0800, Yaodong Li wrote:
>> 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.
>>
> It's literally an entity called WOPCM, which is partitioned and one of
> the partitions is used for GuC. I don't see how many more resons you
> need for intel_wopcm prefix, struct intel_wopcm_partition abstraction
> and struct intel_wopcm_partition instance for GuC?
>
> Why would we try to make the naming scheme to imply something else,
> it'll make the developer's life harder when trying to look at it. I had
> to go look at the spec to make any sense of this, so let's try to avoid
> that for the next developer.
Actually I started the patch with both intel_wopcm and intel_wopcm_partition
defined and implemented. The more I think about it the more I felt
it makes sense to keep the code within GuC level since the main work of
these code is quite simple - trying to find valid values for GuC wopcm.
that's the reason why I renamed it from intel_wopcm_partition to 
intel_guc_wopcm
because with intel_wopcm_partition defined, it will be confusing that we
only have intel_wopcm_partition defined for GuC.

Sorry for the naming thing. It kept changing along with the code 
reviewing, one
reason probably because I really wanted to catch up with all these good 
ideas
and provide a solid solution to this problem.

Again these are my own understanding. If you still think we should use a
intel_wopcm.c|h for these changes. I definitely will respect it and try 
to make things
happen in that way :-) I will hold the submission until we agree on the 
code structure.

Regards,
-Jackie
>
> Regards, Joonas