[Intel-gfx] [PATCH v11 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Yaodong Li]

On 03/01/2018 04:56 AM, Michal Wajdeczko wrote:
> On Thu, 01 Mar 2018 02:01:36 +0100, Jackie Li <yaodong.li at intel.com> 
> wrote:
>
>
>> +    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. */
>> +    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;
>> +    }
>
> hmm, all above checks are very unlikely, and are also covered by the
> next check below, so maybe we can drop them?
These checks make sure these values are in a known range. so that we
don't need to worry about wrap around issues. e.g.
guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd may
wrap around if we don't have (guc_wopcm_base + ctx_rsvd) >= wopcm->size.
>
>> +
>> +    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.
>> +     */
>> +    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);
>> +
>
> bikeshed: [n, m) notation suggests range n..m, so maybe better to use
>
>     DRM_DEBUG_DRIVER("Calculated GuC WOPCM: base %uKiB size %uKiB\n",
I'd like to keep it as [n, m) to suggest it's a region and to align with 
other comments
in the code.
>
>> +    /*
>> +     * GuC WOPCM size needs to be big enough to include all GuC 
>> firmware,
>> +     * extra 8KiB stack for GuC firmware and GUC_WOPCM_RESERVED.
>> +     */
>> +    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);
>
> hmm, maybe to simplify calculations (and match them directly with 
> diagram)
> we should introduce guc_wopcm_size_min:
>
> guc_wopcm_size_min = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED + 
> guc_fw_size;
> if (guc_wopcm_size_min > guc_wopcm_size) {
>     DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
>         guc_wopcm_size_min/1024, guc_wopcm_size/1024);
>
As we discussed, we need find the maximum size value to meet the HW 
requirement. So maybe
I need pass this comment as well.:-)
>> +        return -E2BIG;
>> +    }
>> +
>> +    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size);
>> +    if (err) {
>> +        DRM_ERROR("Failed to meet HW restriction.\n");
>> +        return err;
>> +    }
>> +
>> +    wopcm->guc.base = guc_wopcm_base;
>> +    wopcm->guc.size = guc_wopcm_size;
>> +
>> +    return 0;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.h 
>> b/drivers/gpu/drm/i915/intel_wopcm.h
>> new file mode 100644
>> index 0000000..39d4847
>> --- /dev/null
>> +++ 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.
>
> bikeshed: maybe better to place this doc would be inside struct
> to do not mix documentation style ?
Prefer to keep as it is now:-)
>
>> + * @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;
>> +    } guc;
>> +};
>> +
>> +void intel_wopcm_init_early(struct intel_wopcm *wopcm);
>> +int intel_wopcm_init(struct intel_wopcm *wopcm);
>> +
>> +#endif
>
> only few bikesheds plus one suggestion, with that:
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
Thanks for the review.
-Jackie