[Intel-gfx] [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
Sagar Arun Kamble
sagar.a.kamble at intel.com
Wed Nov 1 14:39:39 UTC 2017
On 11/1/2017 7:54 PM, Michal Wajdeczko wrote:
> 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.
Sure.
>
>>> + 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..
Wanted either upload (except for xfer in "dma xfer") or xfer everywhere.
Can be done in separate patch I guess.
>
>> s/guc_ucode_xfer_rsa/guc_ucode_upload_rsa
>
> guc_mmio_xfer_rsa ?
This looks good.
>
>> s/guc_ucode_xfer_dma/dma_upload_guc_ucode (DMA can xfer GuC/HuC)
>
> guc_dma_xfer_ucode ?
This too looks good.
>
>>> - /* 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