Re: [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx