| Message ID | 20171006090148.53596-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@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. > + } > + 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;
On Fri, 06 Oct 2017 12:43:10 +0200, Sagar Arun Kamble <sagar.a.kamble@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@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@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;
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@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@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@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@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;
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; + } + 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). */ - 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;
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@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@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(-)