[Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Yaodong Li]

On 11/07/2017 02:52 AM, Joonas Lahtinen wrote:
> On Fri, 2017-11-03 at 17:18 -0700, Jackie Li wrote:
>> Static WOPCM partitioning would lead to GuC loading failure
>> if the GuC/HuC firmware size exceeded current static 512KB
>> partition boundary.
>>
>> This patch enabled the dynamical calculation of the WOPCM aperture
>> used by GuC/HuC firmware. GuC WOPCM offset was set to
>> HuC size + reserved WOPCM size. GuC WOPCM size was set to
>> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>>
>> Signed-off-by: Jackie Li <yaodong.li at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: John Spotswood <john.a.spotswood at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
> <SNIP>
>
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>>   	WARN_ON(!list_empty(&dev_priv->contexts.list));
>>   }
>>   
>> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> Use "struct drm_i915_private *i915" when introducing new code that is
> not fiddling with MMIOs.
Will update it.
>> +{
>> +	struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
>> +
>> +	wopcm->size = WOPCM_DEFAULT_SIZE;
>> +
>> +	DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
>> +}
>> +
>>   static int i915_load_modeset_init(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
>>   	if (ret)
>>   		goto cleanup_irq;
>>   
>> +	/*
>> +	 * Get the wopcm memory info.
>> +	 * NOTE: this need to be called before init FW.
>> +	 */
> Useless comments. Comments should always answer question "why?", not
> "what?". And "why?" only needs answering when the code is more obscure
> than this. So when the code reads clearly and you don't need comments
> inside the function bodies.
Will remove it.
>> +	i915_wopcm_init(dev_priv);
>> +
>>   	intel_uc_init_fw(dev_priv);
> WOPCM is the reserved memory for the uC's. Why couldn't it be inside
> the *_uc_* functions? It's only relevant when you have the uCs.
They are related, but different concepts. WOPCM will be there no
matter whether we use uC or not. On the other hand, we need get
the wopcm size before fetching the FW and creating kernel context
which means intel_uc_init_fw would be the best place in uC code to
call this init function. in this case it will be called no matter 
whether uC's
active or not. I think it makes more sense to merge it into ggtt/stolen 
init.
(or would drop this call completely if possible.)
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>>    * This is a wrapper to create an object for use with the GuC. In order to
>>    * use it inside the GuC, an object needs to be pinned lifetime, so we allocate
>>    * both some backing storage and a range inside the Global GTT. We must pin
>> - * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
>> + * it in the GGTT somewhere other than than [0, guc wopcm_top) because that
>>    * range is reserved inside GuC.
>>    *
>>    * Return:	A i915_vma if successful, otherwise an ERR_PTR.
>> @@ -358,7 +358,8 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>>   		goto err;
>>   
>>   	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
>> -			   PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
>> +			   PIN_GLOBAL | PIN_OFFSET_BIAS |
>> +			   intel_guc_wopcm_top(dev_priv));
> Could read just as "guc->wopcm.top"? Because we're not going to change
> it runtime, we could avoid a function. It's a one-off programmable
> register after all.
Good idea! The reason to put it into a function was to do an assert
to make sure wopcm partition was initialized with valid offset, size values.
However, we would never get here if wopcm partition initialization failed,
so will update the code to access the value directly.
>> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>>   
>>   u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
>>   {
>> -	u32 wopcm_size = GUC_WOPCM_TOP;
>> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>>   
>> -	/* On BXT, the top of WOPCM is reserved for RC6 context */
>> -	if (IS_GEN9_LP(dev_priv))
>> -		wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
>> +	GEM_BUG_ON(!wp->guc_wopcm_size);
>>   
>> -	return wopcm_size;
>> +	return wp->guc_wopcm_size;
>> +}
>> +
>> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
>> +{
>> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> +	GEM_BUG_ON(!dev_priv->wopcm.size);
>> +
>> +	return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
>> +}
>> +
>> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
>> +{
>> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> +	GEM_BUG_ON(!wp->guc_wopcm_size);
>> +
>> +	return wp->guc_wopcm_offset;
>> +}
>> +
>> +/*
>> + * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
>> + * which is reserved for Boot ROM, SRAM and WOPCM. All gfx objects
>> + * used by GuC is pinned with PIN_OFFSET_BIAS along with top of WOPCM.
>> + */
>> +u32 guc_ggtt_offset(struct i915_vma *vma)
>> +{
>> +	struct drm_i915_private *dev_priv = vma->vm->i915;
>> +	u32 offset = i915_ggtt_offset(vma);
>> +
>> +	GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
>> +	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
>> +	return offset;
>>   }
> Function prefixes and parameters are not following the convention.
> There also needs to be proper kerneldoc for newly exposed functions.
>
> Then, to the big question, why not address this the way of just once
> calculating the hole? Then simply using the calculated values just like
> *stolen_reserved* properties.
>
> Apart from the high level questions, please sync with somebody local to
> your office about the i915 coding style conventions, it'll more
> efficient to go through rest of the code in more interactive manner for
> a better learning experience.
>
> We should try to avoid making even the simplest operations a function
> call (especially one that is not inline). And as far as I understand,
> the register can be written exactly once so we are not expecting that
> dynamic operation.
>
> While going through the code, please also implement the feature of
> reading the WOPCM register and going with that value, if it's been
> written to already when driver is initializing. We could be the second
> driver initialization after 'rmmod i915' for example.
Actually, I was trying to following the original coding style here :o)
(e.g. there were functions such as intel_guc_wopcm_size in original
i915 code).

As for the reason for using function calls, I thought it will be safer to
have an assertion before returning these values. However, we do have
reasons not to do such a check (since these functions won't even be called
after guc initialization failure, expect for wopcm_top which we need it 
in kernel
context creation). so I probably rework the patch to remove unnecessary
use of function calls in v2.

Regarding the inline, two reasons didn't use inline functions:
a) Wanted to be consistent with existing code convention.
b) No way to use *static inline* to implement this in intel_guc.h since
     they need to access dev_priv fields. This can be solve by moving
     guc wopcm partition into intel_guc. However, it's still a problem for
     guc_gtt_offset if we still want to do GEM_BUG_ON(offset < wopcm top).
     Any suggestions here?

I will definitely triple check with an expert for more feedbacks regarding
the i915 coding style and convention. it's always a pleasure to receive 
valuable
feedbacks and new knowledges :-)

Thanks for the comments and explanation!
>
> Regards, Joonas