Re: [PATCH 9/9] drm/i915/uc: Unify uC firmware upload

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:48)
> The way we load the firmwares is the same for both GuC and HuC, the only
> difference is in the wopcm destination address and the dma flags, so we
> easily can move the logic to a common function and pass in offset and
> flags. The only other difference in the uplaod path are some the extra
> steps that guc does before and after the xfer, but those don't require
> the guc fw to be pinned in ggtt and can safely be performed before
> calling the uc_upload function.
> 
> Note that this patch re-introduces the dma xfer wait for guc loading that
> was removed with "drm/i915/guc: Propagate the fw xfer timeout". This is
> not going to slow us down on a successful load (the dma has to complete
> before fw init can start), but could slightly increase the timeout in case
> of a fw init error.

Should not be a problem if all is well. And if not, picking up on an
error earlier is beneficial.

> +static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
> +                     u32 wopcm_offset, u32 dma_flags)
> +{
> +       struct intel_uncore *uncore = gt->uncore;
> +       u64 offset;
> +       int ret;
> +
> +       intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> +
> +       /* Set the source address for the uCode */
> +       offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
> +       GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
> +       intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));

You've taken an explicit fw, so these can all be _fw.

> +       intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
> +
> +       /* Set the DMA destination */
> +       intel_uncore_write(uncore, DMA_ADDR_1_LOW, wopcm_offset);
> +       intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> +       /*
> +        * Set the trasfer size. The header plus uCode will be copied to WOPCM
> +        * via DMA, excluding any other components
> +        */
> +       intel_uncore_write(uncore, DMA_COPY_SIZE,
> +                          uc_fw->header_size + uc_fw->ucode_size);
> +
> +       /* Start the DMA */
> +       intel_uncore_write(uncore, DMA_CTRL,
> +                          _MASKED_BIT_ENABLE(dma_flags | START_DMA));
> +
> +       /* Wait for DMA to finish */
> +       ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
> +       if (ret)
> +               DRM_ERROR("DMA for %s fw failed, err=%d\n",
> +                         intel_uc_fw_type_repr(uc_fw->type), ret);
> +
> +       /* Disable the bits once DMA is over */
> +       intel_uncore_write(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(dma_flags));
> +
> +       intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> +
> +       return ret;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx