Re: [PATCH v9 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li <yaodong.li@intel.com>]

On 02/09/2018 09:24 AM, Michal Wajdeczko wrote:
>> +    struct intel_guc *guc =
>> +        container_of(guc_wopcm, struct intel_guc, wopcm);
>
> If you define this as "wopcm_to_guc" inline helper, then maybe
> all your functions could take "wopcm" as first parameter?
First thought is this is only one time work, so wanted to keep the code
simple. The following patch added an inline for this when there are more
places needs to use wopcm_to_guc.
>
>> +    u32 reserved = guc_wopcm_context_reserved_size(guc);
>> +    u32 offset, size, top;
>> +    int err;
>> -    /* On BXT, the top of WOPCM is reserved for RC6 context */
>> -    if (IS_GEN9_LP(i915))
>> -        size -= BXT_GUC_WOPCM_RC6_RESERVED;
>> +    if (!guc_fw_size)
>> +        return -EINVAL;
>
> As there are many reasons to fail, maybe it would be good idea to
> add at least DRM_DEBUG_DRIVER to each failing condition?
>
Honestly. that's my personal preference too:-) but I am trying to catch
up with the coding style to avoid these redundancies.
I thought there are redundant because:
0) this func is called by uc_init which will print error message when
     this failed.
1) there are two types of errors: invalid parameters. or FW sizes are
      too big to fit in WOPCM. the upper layer error message could 
easily reflect
     these info by check the error code.
and more, I now do feel some of these checks are redundant and will
remove them. :-)
>> +
>> +    if (reserved >= WOPCM_DEFAULT_SIZE)
>> +        return -E2BIG;
>
> Do we really need to check this every time in runtime?
> I think we can enforce this as GEM_BUG_ON here or in
> guc_wopcm_context_reserved_size() function.
>
Yes. this is redundant since we are total having control
of this value.
>
>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>> +
>> +/*
>> + * GuC WOPCM starts at 144KB (GUC_WOPCM_RESERVED + 128KB reserved 
>> for GuC
>> + * firmware loading) from GuC WOPCM offset on BXT.
>> + */
>> +#define GEN9_GUC_WOPCM_OFFSET        (144 << 10)
>
> Other BXT specific macro (BXT_GUC_WOPCM_RC6_CTX_RESERVED) was
> defined with BXT_ prefix ... can we use common prefix?
>
BXT suggests this is only for Gen9 LP while Gen9 means it's for all Gen9.:-)

Regards,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx