[Intel-gfx] [PATCH 4/8] drm/i915/guc: Separate WOPCM partitioning from constraints check
Yaodong Li
yaodong.li at intel.com
Fri Mar 23 19:30:54 UTC 2018
On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> In the following patches we're going to support constraints checking on
> an already locked partitioning. Let's structure the code now to allow
> for code reuse and reduce the churn later on.
>
> 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 | 141 +++++++++++++++++++++++++++----------
> 1 file changed, 102 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 50854a6b9493..52841d340002 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
> return 0;
> }
>
> +static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size)
> +{
> + return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED +
> + GUC_WOPCM_STACK_RESERVED, PAGE_SIZE);
> +}
> +
> +static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size)
> +{
> + return huc_fw_size + WOPCM_RESERVED_SIZE;
> +}
> +
> static u32
> gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
> {
> @@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size)
> return additional_size;
> }
>
> -static inline int check_hw_restriction(struct drm_i915_private *i915,
> - u32 guc_wopcm_base, u32 guc_wopcm_size,
> - u32 huc_fw_size)
> +static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size)
inline?
> +{
> + if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) {
> + DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n",
> + huc_fw_size_in_wopcm(huc_fw_size) / 1024,
> + wopcm->guc.base / 1024);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> +static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size)
inline?
> +{
> + if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) {
> + DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
> + huc_fw_size_in_wopcm(guc_fw_size) / 1024,
> + wopcm->guc.size / 1024);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> +static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
inline?
> +{
> + if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) {
> + DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> + wopcm->guc.base / 1024);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> +static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> {
We are repeating the the old discussion over the parameters here.
we wanted to keep wopcm contains either valid (non-zero) values
or to be zero all the time, so that we can make a check such as
if (!wopcm->guc.size) then it's valid. Originally, we needed check this
before accessing it in guc_ggtt_offset. Since we are not doing so
maybe we can change it back in this way, or just keep it as it is now
to continue gaining the benefit that it will continue contains valid data?
> + struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> + u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> u32 size;
>
> if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
> - size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
> - guc_wopcm_size);
> + size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> + wopcm->guc.size);
> if (size)
> goto err;
>
> - size = gen9_size_for_huc_restriction(guc_wopcm_size,
> + size = gen9_size_for_huc_restriction(wopcm->guc.size,
> huc_fw_size);
> if (size)
> goto err;
> @@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
>
> err:
> DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> - guc_wopcm_size / 1024,
> + wopcm->guc.size / 1024,
> size / 1024);
>
> return -E2BIG;
> }
>
> +static bool wopcm_check_components_fit(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);
> + u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
> + u32 ctx_rsvd = context_reserved_size(i915);
> + int err;
> +
> + err = check_huc_fw_fits(wopcm, huc_fw_size);
> + if (err)
> + return err;
> +
> + err = check_guc_fw_fits(wopcm, guc_fw_size);
> + if (err)
> + return err;
> +
> + err = check_ctx_rsvd_fits(wopcm, ctx_rsvd);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int wopcm_guc_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 ctx_rsvd = context_reserved_size(dev_priv);
> +
> + wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> + GUC_WOPCM_OFFSET_ALIGNMENT);
I think we should grow it to the alignment boundary instead of
trim it down.
> +
> + wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> + PAGE_SIZE);
Hmm, what if wopcm->guc.base + ctx_rsvd > wopcm->size? we have
no guarantee on what the value would be after doing an add:)
Plus, we should trim it down instead of growing it up:) since we've already
got the maximum available size with wopcm->size - wopcm->guc.base -
ctx_rsvd.
Plus, No PAGE_SIZE since it's not fixed to 4K.
> +
> + DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> + wopcm->guc.base / 1024,
> + (wopcm->guc.base + wopcm->guc.size) / 1024);
> +
> + return 0;
> +}
> +
> /**
> * intel_wopcm_init() - Initialize the WOPCM structure.
> * @wopcm: pointer to intel_wopcm.
> @@ -163,46 +251,21 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
> */
> int intel_wopcm_init(struct intel_wopcm *wopcm)
> {
> - struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> - u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
> - u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> - u32 ctx_rsvd = context_reserved_size(i915);
> - u32 guc_wopcm_base;
> - u32 guc_wopcm_size;
> - u32 guc_wopcm_rsvd;
> int err;
>
> GEM_BUG_ON(!wopcm->size);
>
> - guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> - GUC_WOPCM_OFFSET_ALIGNMENT);
> - if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> - DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> - guc_wopcm_base / 1024);
> - return -E2BIG;
> - }
> -
> - guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
> - guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
> -
> - DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
> - guc_wopcm_base / 1024, guc_wopcm_size / 1024);
> -
> - guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> - if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
> - DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
> - (guc_fw_size + guc_wopcm_rsvd) / 1024,
> - guc_wopcm_size / 1024);
> - return -E2BIG;
> - }
> + err = wopcm_guc_init(wopcm);
> + if (err)
> + return err;
>
> - err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
> - huc_fw_size);
> + err = wopcm_check_hw_restrictions(wopcm);
> if (err)
> return err;
>
> - wopcm->guc.base = guc_wopcm_base;
> - wopcm->guc.size = guc_wopcm_size;
> + err = wopcm_check_components_fit(wopcm);
Check fits here seems a little bit overkill, especially for the checks
such as check_huc_fw_fit() and check_ctx_rsvd_fit() against the calculated
values at this points since these values are actually calculated by the
facts that
guc_wopcm_base should definitely larger than huc_fw_size since we
calculated it
with huc_fw_size + WOPCM_RESERVED_SIZE which means
check_huc_fw_fits should always returns true
and guc_wopcm_size is based on wopcm->size - wopcm->guc.base - ctx_rsvd
which means check_ctx_rsvd_fit() will always return true for this check
as well.
Regards,
-Jackie
More information about the Intel-gfx
mailing list