[Intel-gfx] [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition
Yaodong Li
yaodong.li at intel.com
Tue Apr 3 01:00:50 UTC 2018
On 03/26/2018 03:04 AM, Michał Winiarski wrote:
> On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote:
>> On 03/23/2018 05:34 AM, Michał Winiarski wrote:
>>> We need GuC to load HuC, but it's also possible for GuC to operate on
>>> its own. We don't know what the size of HuC FW may be, so, not wanting
>>> to make any platform-specific hardcoded guesses, we assume that its size
>>> is 0... Which is a very bad approximation.
>>> This has a very unfortunate consequence - once we've booted with GuC and
>>> no HuC, we'll never be able to load HuC (unless we reboot, since the
>>> registers are locked).
>>> Rather than using unknown values in our computations - let's partition
>>> based on GuC size.
>> we can do this based on known GuC and HuC sizes without
>> any assumption on FW sizes :)
>> please also refer to: https://patchwork.freedesktop.org/series/40541/
> You need to have HuC FW present in the filesystem do do that though.
> (IIUC we still get 0 if it's not there)
Yes. we cannot make any assumption to the status of the FW files as well.
so I think we should expect a system reboot for any FW updates.
>>> We have one HW restriction where we're using HuC size (GuC size needs to
>>> be roughly similar to HuC size - which may be unknown at this point),
>>> luckily, another HW restriction makes it very unlikely to run in any
>>> sort of issues in this case.
>>>
>>> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Jackie Li <yaodong.li at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
>>> 1 file changed, 34 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
>>> index 52841d340002..295d302e97b9 100644
>>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>>> @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
>>> return 0;
>>> }
>>> -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>>> +static inline void
>>> +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
>>> +{
>>> + /*
>>> + * We're growing guc region in the direction of lower addresses.
>>> + * We need to use multiples of base alignment, because it has more
>>> + * strict alignment rules.
>>> + */
>>> + size = DIV_ROUND_UP(size, 2);
>> A bit confused here. Don't we just want to push the base downward for
>> *size* bytes?
> Starting from the following:
>
> +--------------+
> |--------------|
> || ||
> || ||
> || GuC ||
> || region ||
> || ||
> || ||
> || ||
> || ||
> +--------------+
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> +--------------+
>
> We want to grow the whole region by size bytes. Pushing the base downward gives
> us this:
>
> +--------------+
> | Empty |
> | space |
> +--------------+
> || ||
> || ||
> || GuC ||
> || region ||
> || ||
> || ||
> || ||
> || ||
> +--------------+
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> +--------------+
>
> Which leaves less space for HuC (and we're also leaving a bunch of unused space
> in our partitioning).
>
> If we modify both base and size to end up with this:
>
> +--------------+
> |--------------|
> || ||
> || ||
> || GuC ||
> || region ||
> || ||
> || ||
> || ||
> || ||
> || ||
> +--------------+
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> | |
> +--------------+
>
> We're still satisfying the HW restriciton, but we're not wasting any space from
> the top (and also we're leaving more space for HuC).
>
>>> + size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
>> Hmm, I think we only need align it to 4K boundary.
> No - because we're modifying both base (16K alignment) and size (4K
> alignment).
Got it:-) Thanks!
>
>>> +
>>> + wopcm->guc.base -= size;
>>> + wopcm->guc.size += size;
>>> +}
>>> +
>>> +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
>>> {
>>> struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>>> u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
>>> @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>>> size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>>> wopcm->guc.size);
>>> if (size)
>>> - goto err;
>>> + __guc_region_grow(wopcm, size);
>>> size = gen9_size_for_huc_restriction(wopcm->guc.size,
>>> huc_fw_size);
>> Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
>> Once the registers were locked, enable_guc=3 may still fail since
>> huc_fw_size
>> may still large enough to break the restriction we want to enforce that:
>> hw_fw_size + 16KB > guc_fw_size.
> Well - as mentioned in the commit message, since we also have a "dword gap"
> restriction, we're left with pretty large GuC size, which makes this problem
> dissaperar for real-world HuC FW sizes.
>
> But yeah - I agree, we're not able to guarantee this.
Hmm, Still I think we shall not make any assumption on this which
driver has no control. It seems that partitioning based on real FW sizes
+ expecting
system reboot for FW file changes should be a driver side solution to
support
enable_guc=1 then enable_guc=3 case.
>
>>> if (size)
>>> - goto err;
>>> - }
>>> -
>>> - return 0;
>>> + __guc_region_grow(wopcm, size);
>>> -err:
>>> - DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
>>> - wopcm->guc.size / 1024,
>>> - size / 1024);
>>> -
>>> - return -E2BIG;
>>> + GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>>> + wopcm->guc.size));
>>> + GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
>>> + huc_fw_size));
>>> + }
>>> }
>>> static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>>> @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>>> return 0;
>>> }
>>> -static int wopcm_guc_init(struct intel_wopcm *wopcm)
>>> +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
>>> {
>>> struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>>> - u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
>>> + u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
>>> u32 ctx_rsvd = context_reserved_size(dev_priv);
>>> - wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
>>> - GUC_WOPCM_OFFSET_ALIGNMENT);
>>> + wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
>>> - wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
>>> - PAGE_SIZE);
>>> -
>>> - DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
>>> - wopcm->guc.base / 1024,
>>> - (wopcm->guc.base + wopcm->guc.size) / 1024);
>>> + wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
>>> + GUC_WOPCM_OFFSET_ALIGNMENT);
>> guc.size + ctx_rsvd > wopcm->size check?
> It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit).
>
Hmm, Just worried about wopcm->size - ctx_rsvd - wopcm->guc.size
may be incorrect (wrap around) since we don't have any checks on
the value of ctx_rsvd + wopcm->guc.size (it might be larger than wopcm
size since
the checks we've done before could only guarantee wopcm->guc.size <
wopcm.size).
Regards,
-Jackie
More information about the Intel-gfx
mailing list