Re: [PATCH 05/15] drm/i915: GuC-specific firmware loader

Yu Dai
2015-06-18T17:55:54Z

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