On 06/15/2015 01:30 PM, Chris Wilson wrote:
On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
----snip----
> + * Return true if get a success code from normal boot or RC6 boot
> + */
> +static inline bool i915_guc_get_status(struct drm_i915_private *dev_priv,
> + u32 *status)
> +{
> + *status = I915_READ(GUC_STATUS);
> + return (((*status) & GS_UKERNEL_MASK) == GS_UKERNEL_READY ||
> + ((*status) & GS_UKERNEL_MASK) == GS_UKERNEL_LAPIC_DONE);
Weird function. Does two things, only one of those is get_status. Maybe
you would like to split this up better and use a switch when you mean a
switch. Or rename it to reflect it's use only as a condition.
Yes. It makes sense to change it to something like
i915_guc_is_ucode_loaded().
> +}
> +
> +/* Transfers the firmware image to RAM for execution by the microcontroller.
> + *
> + * GuC Firmware layout:
> + * +-------------------------------+ ----
> + * | CSS header | 128B
> + * +-------------------------------+ ----
> + * | uCode |
> + * +-------------------------------+ ----
> + * | RSA signature | 256B
> + * +-------------------------------+ ----
> + * | RSA public Key | 256B
> + * +-------------------------------+ ----
> + * | Public key modulus | 4B
> + * +-------------------------------+ ----
> + *
> + * Architecturally, the DMA engine is bidirectional, and in can potentially
> + * even transfer between GTT locations. This functionality is left out of the
> + * API for now as there is no need for it.
> + *
> + * Be note that GuC need the CSS header plus uKernel code to be copied as one
> + * chunk of data. RSA sig data is loaded via MMIO.
> + */
> +static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv)
> +{
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> + struct drm_i915_gem_object *fw_obj = guc_fw->uc_fw_obj;
> + unsigned long offset;
> + struct sg_table *sg = fw_obj->pages;
> + u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)];
> + int i, ret = 0;
> +
> + /* uCode size, also is where RSA signature starts */
> + offset = ucode_size = guc_fw->uc_fw_size - UOS_CSS_SIGNING_SIZE;
> +
> + /* Copy RSA signature from the fw image to HW for verification */
> + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE, offset);
> + for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++)
> + I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]);
> +
> + /* Set the source address for the new blob */
> + offset = i915_gem_obj_ggtt_offset(fw_obj);
Why would it even have a GGTT vma? There's no precondition here to
assert that it should.
It is pinned into GGTT inside gem_allocate_guc_obj.
> + I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> + I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
> +
> + /* Set the destination. Current uCode expects an 8k stack starting from
> + * offset 0. */
> + I915_WRITE(DMA_ADDR_1_LOW, 0x2000);
> +
> + /* XXX: The image is automatically transfered to SRAM after the RSA
> + * verification. This is why the address space is chosen as such. */
> + I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> + I915_WRITE(DMA_COPY_SIZE, ucode_size);
> +
> + /* Finally start the DMA */
> + I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
> +
Just assuming that the writes land and in the order you expect?
A POSTING_READ of DMA_COPY_SIZE before issue the DMA is enough here? Or,
POSTING_READ all those writes?
-Alex
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
