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

[Sagar Arun Kamble]

On 10/6/2017 5:17 PM, Michal Wajdeczko wrote:
> 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
Yes. We can settle for 3 for now and revisit if need to update arises.
Patch looks good to me.

Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>
>>> +    }
>>> +    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;