[Intel-gfx] [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
Michal Wajdeczko
michal.wajdeczko at intel.com
Wed Nov 1 14:24:10 UTC 2017
On Mon, 30 Oct 2017 15:00:52 +0100, Sagar Arun Kamble
<sagar.a.kamble at intel.com> wrote:
>
>
> On 10/27/2017 10:45 PM, Michal Wajdeczko wrote:
>> Transfer of GuC firmware requires few steps that currently
>> are implemented in two large functions. Split this code into
>> smaller functions to make these steps small and clear.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_guc_fw.c | 173
>> ++++++++++++++++++++++--------------
>> 1 file changed, 106 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index ef67a36..2a10bcf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>> @@ -97,23 +97,53 @@ int intel_guc_fw_select(struct intel_guc *guc)
>> return 0;
>> }
>> -/*
>> - * Read the GuC status register (GUC_STATUS) and store it in the
>> - * specified location; then return a boolean indicating whether
>> - * the value matches either of two values representing completion
>> - * of the GuC boot process.
>> - *
>> - * This is used for polling the GuC status in a wait_for()
>> - * loop below.
>> - */
>> -static inline bool guc_ucode_response(struct drm_i915_private
>> *dev_priv,
>> - u32 *status)
>> +static void guc_ucode_xfer_prepare(struct drm_i915_private *dev_priv)
>> {
>> - u32 val = I915_READ(GUC_STATUS);
>> - u32 uk_val = val & GS_UKERNEL_MASK;
>> - *status = val;
>> - return (uk_val == GS_UKERNEL_READY ||
>> - ((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE));
>> + /* Enable MIA caching. GuC clock gating is disabled. */
> Clock gating comment is linked with below WaDisableMinuteIa*. Can you
> update in this patch. Better to drop with Wa name telling the intent.
>> + I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
>> +
>> + /* WaDisableMinuteIaClockGating:bxt */
>> + if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
>> + I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
>> + ~GUC_ENABLE_MIA_CLOCK_GATING));
>> + }
>> +
>> + /* WaC6DisallowByGfxPause:bxt */
>> + if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
>> + I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
>> +
>> + if (IS_GEN9_LP(dev_priv))
>> + I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
>> + else
>> + I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
>> +
>> + if (IS_GEN9(dev_priv)) {
>> + /* DOP Clock Gating Enable for GuC clocks */
>> + I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
>> + I915_READ(GEN7_MISCCPCTL)));
>> +
>> + /* allows for 5us (in 10ns units) before GT can go to RC6 */
>> + I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
>> + }
>> +}
>> +
>> +/* Copy RSA signature from the fw image to HW for verification */
>> +static int guc_ucode_xfer_rsa(struct intel_uc_fw *guc_fw, struct
>> i915_vma *vma)
>> +{
>> + struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> + struct sg_table *sg = vma->pages;
>> + u32 rsa[UOS_RSA_SCRATCH_MAX_COUNT];
>> + int i;
>> +
>> + if (sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa),
>> + guc_fw->rsa_offset) != sizeof(rsa))
>> + return -EINVAL;
>> +
>> + for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
>> + I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
>> +
>> + return 0;
>> }
>> /*
>> @@ -122,29 +152,19 @@ static inline bool guc_ucode_response(struct
>> drm_i915_private *dev_priv,
>> * Architecturally, the DMA engine is bidirectional, and can
>> potentially even
>> * transfer between GTT locations. This functionality is left out of
>> the API
>> * for now as there is no need for it.
>> - *
>> - * Note that GuC needs the CSS header plus uKernel code to be copied
>> by the
>> - * DMA engine in one operation, whereas the RSA signature is loaded
>> via MMIO.
>> */
>> -static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>> - struct i915_vma *vma)
>> +static int guc_ucode_xfer_dma(struct intel_uc_fw *guc_fw, struct
>> i915_vma *vma)
>> {
>> - struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> + struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> unsigned long offset;
>> - struct sg_table *sg = vma->pages;
>> - u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
>> - int i, ret = 0;
>> -
>> - /* where RSA signature starts */
>> - offset = guc_fw->rsa_offset;
>> -
>> - /* Copy RSA signature from the fw image to HW for verification */
>> - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa), offset);
>> - for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
>> - I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
>> + u32 status;
>> + int ret;
>> - /* The header plus uCode will be copied to WOPCM via DMA, excluding
>> any
>> - * other components */
>> + /*
>> + * 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);
>> /* Set the source address for the new blob */
>> @@ -162,26 +182,55 @@ static int guc_ucode_xfer_dma(struct
>> drm_i915_private *dev_priv,
>> /* Finally start the DMA */
>> I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
>> + /* Wait for DMA to finish */
>> + ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, START_DMA, 0,
>> + 2, 100, &status);
>> + DRM_DEBUG_DRIVER("GuC DMA status %#x\n", status);
>> +
> Will it be valuable to create dma_response function for this wait? Might
> be able to use in HuC too.
Not yet. And I would rather export intel_guc_dma_xfer_ucode() and do this
in separate patch that will make this function also usable for HuC
transfer.
>> + return ret;
>> +}
>> +
>> +/*
>> + * Read the GuC status register (GUC_STATUS) and store it in the
>> + * specified location; then return a boolean indicating whether
>> + * the value matches either of two values representing completion
>> + * of the GuC boot process.
>> + *
>> + * This is used for polling the GuC status in a wait_for()
>> + * loop below.
>> + */
>> +static inline bool guc_ucode_response(struct intel_guc *guc, u32
>> *status)
>> +{
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> + u32 val = I915_READ(GUC_STATUS);
>> + u32 uk_val = val & GS_UKERNEL_MASK;
>> +
>> + *status = val;
>> + return (uk_val == GS_UKERNEL_READY) ||
>> + ((val & GS_MIA_CORE_STATE) && (uk_val == GS_UKERNEL_LAPIC_DONE));
>> +}
>> +
>> +static int guc_ucode_wait(struct intel_guc *guc)
>> +{
>> + u32 status;
>> + int ret;
>> +
>> /*
>> - * Wait for the DMA to complete & the GuC to start up.
>> + * Wait for the GuC to start up.
>> * NB: Docs recommend not using the interrupt for completion.
>> * Measurements indicate this should take no more than 20ms, so a
>> * timeout here indicates that the GuC has failed and is unusable.
>> * (Higher levels of the driver will attempt to fall back to
>> * execlist mode if this happens.)
>> */
>> - ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
>> -
>> - DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
>> - I915_READ(DMA_CTRL), status);
>> + ret = wait_for(guc_ucode_response(guc, &status), 100);
>> + DRM_DEBUG_DRIVER("GuC status %#x\n", status);
>> if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>> DRM_ERROR("GuC firmware signature verification failed\n");
>> ret = -ENOEXEC;
>> }
>> - DRM_DEBUG_DRIVER("returning %d\n", ret);
>> -
>> return ret;
>> }
>> @@ -198,34 +247,24 @@ static int guc_ucode_xfer(struct intel_uc_fw
>> *guc_fw, struct i915_vma *vma)
>> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> - /* Enable MIA caching. GuC clock gating is disabled. */
>> - I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
>> -
>> - /* WaDisableMinuteIaClockGating:bxt */
>> - if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
>> - I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
>> - ~GUC_ENABLE_MIA_CLOCK_GATING));
>> - }
>> -
>> - /* WaC6DisallowByGfxPause:bxt */
>> - if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
>> - I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
>> -
>> - if (IS_GEN9_LP(dev_priv))
>> - I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
>> - else
>> - I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
>> + guc_ucode_xfer_prepare(dev_priv);
>> - if (IS_GEN9(dev_priv)) {
>> - /* DOP Clock Gating Enable for GuC clocks */
>> - I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
>> - I915_READ(GEN7_MISCCPCTL)));
>> + /*
>> + * Note that GuC needs the CSS header plus uKernel code to be copied
>> + * by the DMA engine in one operation, whereas the RSA signature is
>> + * loaded via MMIO.
>> + */
>> + ret = guc_ucode_xfer_rsa(guc_fw, vma);
>> + if (ret)
>> + DRM_WARN("GuC firmware signature upload error %d\n", ret);
> Unless there is a need that I am unaware of, can we rename as:
> s/guc_ucode_xfer/guc_ucode_upload
Note that intel_uc_fw_upload() takes "xfer" func pointer as parameter..
> s/guc_ucode_xfer_rsa/guc_ucode_upload_rsa
guc_mmio_xfer_rsa ?
> s/guc_ucode_xfer_dma/dma_upload_guc_ucode (DMA can xfer GuC/HuC)
guc_dma_xfer_ucode ?
>> - /* allows for 5us (in 10ns units) before GT can go to RC6 */
>> - I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
>> - }
>> + ret = guc_ucode_xfer_dma(guc_fw, vma);
>> + if (ret)
>> + DRM_WARN("GuC firmware upload error %d\n", ret);
>> - ret = guc_ucode_xfer_dma(dev_priv, vma);
>> + ret = guc_ucode_wait(guc);
>> + if (ret)
>> + DRM_ERROR("GuC firmware error %d\n", ret);
>> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>
More information about the Intel-gfx
mailing list