Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Tue, 13 Feb 2018 22:50:53 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 02/13/2018 08:06 AM, Michal Wajdeczko wrote:
>> +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,
>>> +                    u32 huc_fw_size)
>>> +{
>>> +    /*
>>> +     * On Gen9 & CNL A0, hardware requires the total available GuC  
>>> WOPCM
>>> +     * size to be larger than or equal to HuC firmware size.  
>>> Otherwise,
>>> +     * firmware uploading would fail.
>>> +     */
>>> +    if (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)
>>
>> What if guc_wopcm->size < GUC_WOPCM_RESERVED ?
>>
> we've already had following check before this. which had guaranteed
> guc_wopcm->size >= guc_fw_size + reserved, thus,
> guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED
> so, guc_wopcm->size > GUC_WOPCM_RESERVED:-)
>
>      reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
>      if ((guc_fw_size + reserved) > guc_wopcm->size) {
>          DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
>          return -E2BIG;
>      }

Hmm, that only proves that current partitioning code is too complicated :)
If you look at diagram it should be possible to implement it as

guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
reserved = context_reserved_size(i915);

if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
	return -E2BIG;

(E&O)

>>> +        return -E2BIG;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static inline int gen9_check_dword_gap(struct intel_guc_wopcm  
>>> *guc_wopcm)
>>>  {
>>>      u32 guc_wopcm_start;
>>> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct  
>>> intel_guc_wopcm *guc_wopcm)
>>>      return 0;
>>>  }
>>> -static inline int guc_wopcm_size_check(struct intel_guc *guc)
>>> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32  
>>> huc_fw_size)
>>>  {
>>>      struct drm_i915_private *i915 = guc_to_i915(guc);
>>>      struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
>>> +    int err = 0;
>>>     if (IS_GEN9(i915))
>>> -        return gen9_check_dword_gap(guc_wopcm);
>>> +        err = gen9_check_dword_gap(guc_wopcm);
>>> -    return 0;
>>> +    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0,  
>>> CNL_REVID_A0))
>>> +        err = check_huc_fw_fits(guc_wopcm, huc_fw_size);
>>
>> Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits()  
>> passes ?
>>
> oops! will fix this.:-)
>>> +
>>> +    return err;
>>>  }
>>> /**
>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
>>> *guc_wopcm, u32 guc_fw_size,
>>>      guc->wopcm.size = size;
>>>      guc->wopcm.top = top;
>>> -    err = guc_wopcm_size_check(guc);
>>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>>      if (err) {
>>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>>          return err;
>>
>> I'm more and more convinced that we should use "intel_wopcm" to make
>> partition and all these checks
>>
> These are all GuC wopcm related, it only checks guc wopcm size. so I am  
> wondering whether
> I am still misunderstanding anything here?since the purpose of these  
> calculation and checks are
> clearly all GuC related. To be more precisely GuC DMA related. The  
> driver's responsibility is to
> calculate the GuC DMA offset and GuC wopcm size values based on guc/huc  
> fw sizes and
> all these checks are all for the correctness for the GuC  wopcm size.
> I don't see any reason why these should be a part of global intel_wopcm  
> even if we really
> want to keep something like wopcm_regions for both guc/huc(though I  
> still don't know what
> the benefit real is to keep something like HuC wopcm region except for  
> sth like debugfs output?).
> even in this case, we still at least keep these as a part of  GuC since  
> we really need it to setup
> GuC HW :- Or do you mean we should create an intel_wopcm to carry info  
> such as
> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a  
> little bit confused here:-(

struct intel_wopcm should carry only results of WOPCM partitioning between
all agents including GuC. There is no need to carry fw sizes as those are
only needed as input for calculations.

You can still program GuC region from uc code.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx