Quoting Jackie Li (2018-03-02 02:16:42)
> +/**
> + * intel_guc_init_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
> + * @guc: intel_guc structure.
> + *
> + * This functional will calculate and initialize the ggtt_pin_bias value based
s/functional/function/
> + * on overall WOPCM size and GuC WOPCM size.
> + */
> +void intel_guc_init_ggtt_pin_bias(struct intel_guc *guc)
<SNIP>
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -49,6 +49,9 @@ struct intel_guc {
> struct intel_guc_log log;
> struct intel_guc_ct ct;
>
> + /** @ggtt_pin_bias: offset where Non-WOPCM memory starts. */
I'm pretty sure /** description */ should be enough. If we really should
have this in the doxygen is the bigger question, maybe make it a normal
comment like the surrounding ones, for the time being.
> + u32 ggtt_pin_bias;
> +
> /* Log snapshot if GuC errors during load */
> struct drm_i915_gem_object *load_err_log;
>
> @@ -108,10 +111,11 @@ static inline void intel_guc_notify(struct intel_guc *guc)
> * @guc: intel_guc structure.
> * @vma: i915 graphics virtual memory area.
> *
> - * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
> - * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is
> - * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
> - * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
> + * GuC does not allow any gfx GGTT address that falls into range
> + * [0, GuC ggtt_pin_bias), which is reserved for Boot ROM, SRAM and WOPCM.
> + * Currently, in order to exclude [0, GuC ggtt_pin_bias) address space from
> + * GGTT, all gfx objects used by GuC is allocated with intel_guc_allocate_vma()
s/is allocated/are allocated/
> + * and pinned with PIN_OFFSET_BIAS along with the value of GuC ggtt_pin_bias.
> *
> * Return: GGTT offset that meets the GuC gfx address requirement.
This gives an impression that something is done for the vma to make it
comply, instead of just checking it complies. Is that going to be the
case in future? If not, "Return: GGTT offset of the vma" would be more
correct.
> @@ -129,6 +133,7 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
> void intel_guc_init_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> void intel_guc_init_params(struct intel_guc *guc);
> +void intel_guc_init_ggtt_pin_bias(struct intel_guc *guc);
Bit unclear why this amount of details about the initialization sequence get
exposed to header files. Lets fix this in the later series.
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -207,7 +207,7 @@ int intel_huc_auth(struct intel_huc *huc)
> return -ENOEXEC;
>
> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_OFFSET_BIAS | guc->ggtt_pin_bias);
Indent should be fixed while here.
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -115,6 +115,22 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
> return uc_fw->path != NULL;
> }
>
> +/**
> + * intel_uc_fw_get_upload_size()() - Get size of firmware needed to be uploaded.
I don't think the "()()" is correct or needed.
> + * @uc_fw: uC firmware.
> + *
> + * Get the size of the firmware that will be uploaded to WOPCM.
"firmware and header"?
> + *
> + * Return: Upload firmware size, or zero on firmware fetch failure.
> + */
> +static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
<SNIP>
> +static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 guc_wopcm_size)
> +{
> + u32 offset;
> +
> + /*
> + * GuC WOPCM size shall be at least a dword larger than the offset from
> + * WOPCM base (GuC WOPCM offset from WOPCM base + GEN9_GUC_WOPCM_OFFSET)
> + * due to hardware limitation on Gen9.
> + */
> + offset = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET;
> + if (offset > guc_wopcm_size ||
> + (guc_wopcm_size - offset) < sizeof(u32)) {
> + DRM_ERROR("GuC WOPCM size (%uKiB) is too small.%uKiB needed.\n",
Drop the () and add " " after .
> + guc_wopcm_size / 1024,
> + (u32)(offset + sizeof(u32)) / 1024);
> + return -E2BIG;
> + }
> +
> + return 0;
> +}
> +
> +static inline int check_hw_restriction(struct drm_i915_private *i915,
> + u32 guc_wopcm_base, u32 guc_wopcm_size)
> +{
> + int err = 0;
> +
> + if (IS_GEN9(i915))
> + err = gen9_check_dword_gap(guc_wopcm_base, guc_wopcm_size);
> + if (err)
> + return err;
You can just "return err;" and the extra checks would become:
if (!err && IS_FOO())
err = ...;
> +
> + return 0;
> +}
> +
> +/**
> + * intel_wopcm_init() - Initialize the WOPCM structure.
> + * @wopcm: pointer to intel_wopcm.
> + *
> + * This function will partition WOPCM space based on GuC and HuC firmware sizes
> + * and will allocate max remaining for use by GuC. This function will also
> + * enforce platform dependent hardware restrictions on GuC WOPCM offset and
> + * size. It will fail the WOPCM init if any of these checks were failed, so that
> + * the following GuC firmware uploading would be aborted.
> + *
> + * Return: 0 on success, non-zero error code on failure.
> + */
> +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);
> +
> + if (guc_fw_size >= wopcm->size) {
> + DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.",
> + guc_fw_size / 1024);
> + return -E2BIG;
> + }
> +
> + if (huc_fw_size >= wopcm->size) {
> + DRM_ERROR("HuC FW (%uKiB) is too big to fit in WOPCM.",
> + huc_fw_size / 1024);
> + return -E2BIG;
> + }
> +
> + /* Hardware requires GuC WOPCM base to be 16K aligned. */
Rather spurious comment, especially as it hardcodes the value that
is nicely de-magicalized with a #define ;)
> + 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 must be multiple of 4K pages. We've got the maximum
> + * WOPCM size available for GuC. Trim the size value to 4K boundary.
> + */
Ditto.
> + 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 size needs to be big enough to include all GuC firmware,
> + * extra 8KiB stack for GuC firmware and GUC_WOPCM_RESERVED.
> + */
This comment can be read from the code.
> + 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 = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size);
> + if (err) {
> + DRM_ERROR("Failed to meet HW restriction.\n");
We already get DRM_ERROR for all cases in check_hw_restriction, so this
should not be needed.
> +++ b/drivers/gpu/drm/i915/intel_wopcm.h
> @@ -0,0 +1,34 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2017-2018 Intel Corporation
> + */
> +
> +#ifndef _INTEL_WOPCM_H_
> +#define _INTEL_WOPCM_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct intel_wopcm - overall WOPCM info and WOPCM regions.
> + * @size: size of overall WOPCM.
> + * @guc: GuC WOPCM Region info.
> + */
> +struct intel_wopcm {
> + u32 size;
> + struct {
> + /**
> + * @base: GuC WOPCM base which is offset from WOPCM base.
> + */
> + u32 base;
> + /**
> + * @size: size of the GuC WOPCM region.
> + */
> + u32 size;
Again, I assume there would be more brief way of writing these comments.
With the typos corrected and spurious comments removed, this is:
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
