[Intel-gfx] [PATCH] drm/i915/guc: Move firmware size check out of generic code

[Michal Wajdeczko]

On Fri, 06 Oct 2017 12:43:10 +0200, Sagar Arun Kamble  
<sagar.a.kamble at intel.com> wrote:

>
>
> On 10/6/2017 2:31 PM, Michal Wajdeczko wrote:
>> Checking GuC firmware size can be done in GuC specific code
>> right before DMA copy as it is unlikely to fail anyway.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc_loader.c | 14 +++++++++++---
>>   drivers/gpu/drm/i915/intel_uc_fw.c      |  8 --------
>>   2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c  
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index c7a800a..f245aa5 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -198,6 +198,7 @@ static int guc_ucode_xfer_dma(struct  
>> drm_i915_private *dev_priv,
>>   	unsigned long offset;
>>   	struct sg_table *sg = vma->pages;
>>   	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
>> +	u32 size = guc_fw->header_size + guc_fw->ucode_size;
>>   	int i, ret = 0;
>>     	/* where RSA signature starts */
>> @@ -208,9 +209,16 @@ static int guc_ucode_xfer_dma(struct  
>> drm_i915_private *dev_priv,
>>   	for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
>>   		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
>>   -	/* The header plus uCode will be copied to WOPCM via DMA, excluding  
>> any
>> -	 * other components */
>> -	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
>> +	/*
>> +	 * The header plus uCode will be copied to WOPCM via DMA, excluding  
>> any
>> +	 * other components. Make sure that firmware fits there.
>> +	 */
>> +	if (unlikely(size > intel_guc_wopcm_size(dev_priv))) {
>> +		DRM_ERROR("GuC: Firmware is too large (%dB) to fit in WOPCM\n",
>> +			  size);
>> +		return -EFBIG;
> Top level function is converting this to -EAGAIN and would be  
> unnecessary to retry in that case.

Hmm, top level function may receive following errors:
-ETIMEDOUT
-ENOEXEC (signature verification failed)
timeout > 0
-EINTR
-EFAULT
-EINVAL
-ENOMEM
-EFBIG (not only from above case)
-EEXIST
-EIO
-ENOSPC
-E2BIG
...
and unconditionally converts all of them.
Note that there are other cases when retry will not help.

So maybe we should:
1) let guc_ucode_xfer[_dma] decide which failed step can be retried
    (by converting any transient error into -EAGAIN)
or,
2) let intel_uc_init_hw decide when to retry based on error codes
    (retry only on EAGAIN EINTR ETIMEDOUT)
or,
3) ignore any unlikely error duplications caused by retry
    (note that today we retry only due to WA)

Michal

>> +	}
>> +	I915_WRITE(DMA_COPY_SIZE, size);
>>     	/* Set the source address for the new blob */
>>   	offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 766b1cb..482115b 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -108,14 +108,6 @@ void intel_uc_fw_fetch(struct drm_i915_private  
>> *dev_priv,
>>   	 */
>>   	switch (uc_fw->type) {
>>   	case INTEL_UC_FW_TYPE_GUC:
>> -		/* Header and uCode will be loaded to WOPCM. Size of the two. */
>> -		size = uc_fw->header_size + uc_fw->ucode_size;
>> -
>> -		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
> This comment was not correct. So removal makes sense.
>> -		if (size > intel_guc_wopcm_size(dev_priv)) {
>> -			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
>> -			goto fail;
>> -		}
>>   		uc_fw->major_ver_found = css->guc.sw_version >> 16;
>>   		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
>>   		break;