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@xxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
