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

From: Yu Dai <yu.dai@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	"Gordon, David S" <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/15] drm/i915: GuC-specific firmware loader
Date: Thu, 18 Jun 2015 10:53:10 -0700	[thread overview]
Message-ID: <55830586.4000501@intel.com> (raw)
In-Reply-To: <20150615203057.GG28462@nuc-i3427.alporthouse.com>


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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx