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

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Yaodong Li <yaodong.li@intel.com>,
	Michal Wajdeczko <michal.wajdeczko@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Subject: Re: [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning
Date: Thu, 14 Dec 2017 13:43:07 +0200	[thread overview]
Message-ID: <1513251787.5244.7.camel@linux.intel.com> (raw)
In-Reply-To: <dd5a89c7-e0a4-b214-aadf-7133e73527b2@intel.com>

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.

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