Re: [PATCH v3 04/15] drm/i915: GuC firmware loader

[Dave Gordon <david.s.gordon@intel.com>]

On 17/04/15 22:21, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> Add GuC firmware loader. It uses the unified firmware loader to
> fetch firmware blob first, then load to hw in driver main thread.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile           |   3 +-
>  drivers/gpu/drm/i915/i915_dma.c         |   6 +
>  drivers/gpu/drm/i915/i915_drv.h         |   7 +
>  drivers/gpu/drm/i915/i915_gem_stolen.c  |  10 +
>  drivers/gpu/drm/i915/i915_reg.h         |   4 +-
>  drivers/gpu/drm/i915/intel_guc.h        | 103 ++++++++++
>  drivers/gpu/drm/i915/intel_guc_loader.c | 348 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc_loader.c  |   3 +
>  drivers/gpu/drm/i915/intel_uc_loader.h  |   4 +
>  9 files changed, 486 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc.h
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c

Hi Alex,

some comments interspersed below ...

> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e44116f..5d50b5b 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -465,6 +465,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_gem:
>  	mutex_lock(&dev->struct_mutex);
> +	intel_guc_ucode_fini(dev);
>  	i915_gem_cleanup_ringbuffer(dev);
>  	i915_gem_context_fini(dev);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -861,6 +862,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_uncore_init(dev);
>  
> +	intel_wopcm_init(dev);
> +
> +	intel_guc_ucode_init(dev);

Can we put this any earlier? We might as well get the async fetch
started as early as possible ...

> +
>  	ret = i915_gem_gtt_init(dev);
>  	if (ret)
>  		goto out_regs;
> @@ -1108,6 +1113,7 @@ int i915_driver_unload(struct drm_device *dev)
>  	flush_workqueue(dev_priv->wq);
>  
>  	mutex_lock(&dev->struct_mutex);
> +	intel_guc_ucode_fini(dev);
>  	i915_gem_cleanup_ringbuffer(dev);
>  	i915_gem_context_fini(dev);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6e8d106..235fc08 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -50,6 +50,7 @@
>  #include <linux/intel-iommu.h>
>  #include <linux/kref.h>
>  #include <linux/pm_qos.h>
> +#include "intel_guc.h"
>  
>  /* General customization:
>   */
> @@ -1581,6 +1582,8 @@ struct drm_i915_private {
>  
>  	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
>  
> +	struct intel_guc guc;
> +
>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>  	 * controller on different i2c buses. */
>  	struct mutex gmbus_mutex;
> @@ -2430,6 +2433,9 @@ struct drm_i915_cmd_table {
>  #define HAS_RC6(dev)		(INTEL_INFO(dev)->gen >= 6)
>  #define HAS_RC6p(dev)		(INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
>  
> +#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev))
> +#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev))
> +
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
> @@ -3008,6 +3014,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  					       u32 stolen_offset,
>  					       u32 gtt_offset,
>  					       u32 size);
> +void intel_wopcm_init(struct drm_device *dev);

See below ...

>  
>  /* i915_gem_shrinker.c */
>  unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..fbf7667 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -551,3 +551,13 @@ err_out:
>  	drm_gem_object_unreference(&obj->base);
>  	return NULL;
>  }
> +
> +void intel_wopcm_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (HAS_GUC_UCODE(dev)) {
> +		I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
> +		I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET);
> +	}
> +}

I don't understand why this function has been added to this file;
it doesn't appear to relate to anything else here, nor does anything
else in this file relate to the GuC.

If it went into i915_dma.c instead, it could be static.

> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h

> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> new file mode 100644
> index 0000000..a999044
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c

[snip]

> +/* Transfers the firmware image to RAM for execution by the microcontroller.
> +
> + * GuC Firmware layout:
> + * +-------------------------------+  ----
> + * |          CSS header           |  128B
> + * +-------------------------------+  ----
> + * |         RSA signature         |  256B
> + * +-------------------------------+  ----
> + * |         RSA public Key        |  256B
> + * +-------------------------------+  ----
> + * |       Public key modulus      |    4B
> + * +-------------------------------+  ----
> + * |             uCode             |
> + * +-------------------------------+  ----
> + *
> + * 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 ucode_xfer_sync(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, *blob;
> +	int i, ret = 0;
> +
> +	/* Copy RSA signature from the fw image to HW for verification */
> +	blob = (uint32_t *)(dev_priv->guc.guc_fw.uc_fw_blob->data
> +			+ UOS_RSA_SIG_OFFSET);
> +	for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(uint32_t); i++)
> +		I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(uint32_t), blob[i]);
> +
> +	/* Re-arrange data per GuC request (CSS header + uCode) */
> +
> +	/* Copy CSS header  */
> +	blob = (uint32_t *)(dev_priv->guc.guc_fw.uc_fw_blob->data
> +			+ UOS_CSS_HEADER_OFFSET);
> +	sg_copy_from_buffer(sg->sgl, sg->nents, blob, UOS_CSS_HEADER_SIZE);
> +
> +	/* Copy ucode */
> +	blob = (uint32_t *)(dev_priv->guc.guc_fw.uc_fw_blob->data
> +			+ UOS_UCODE_OFFSET);
> +	ucode_size = guc_fw->uc_fw_size - UOS_UCODE_OFFSET;
> +	sg_pcopy_from_buffer(sg->sgl, sg->nents, blob,
> +			ucode_size, UOS_CSS_HEADER_SIZE);

I hadn't realised before that the GuC didn't accept the data in the same
format as it comes from the firmware image. And that this code therefore
copies the data from the blob to the GEM object each time the GuC is
reloaded :(

I intended that the raw firmware image should be released once the data
had been copied, so as mentioned in my last email, I think we should
rework the generic firmware loader to let the uC-specific part save the
data in its own preferred format, if that isn't just a copy of the blob.

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_uc_loader.c b/drivers/gpu/drm/i915/intel_uc_loader.c
> index bc499f4..65031ff 100644
> --- a/drivers/gpu/drm/i915/intel_uc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_uc_loader.c
> @@ -77,6 +77,9 @@ static void uc_fw_finish(struct drm_device *dev, struct intel_uc_fw *uc_fw)
>  		uc_fw->uc_fw_blob = fw;
>  	}
>  
> +	if (uc_fw->uc_fw_check && !uc_fw->uc_fw_check(uc_fw))
> +		goto fail;
> +

I'll fold this into the base version of the generic code.

> diff --git a/drivers/gpu/drm/i915/intel_uc_loader.h b/drivers/gpu/drm/i915/intel_uc_loader.h
> index 0994f98..4ed6e94 100644
> --- a/drivers/gpu/drm/i915/intel_uc_loader.h
> +++ b/drivers/gpu/drm/i915/intel_uc_loader.h
> @@ -68,11 +68,15 @@ struct intel_uc_fw {
>  	struct drm_i915_gem_object *	uc_fw_obj;
>  	enum intel_uc_fw_status		uc_fw_fetch_status;
>  	enum intel_uc_fw_status		uc_fw_load_status;
> +	uint32_t			uc_fw_ver_major;
> +	uint32_t			uc_fw_ver_minor;
> +	bool				(*uc_fw_check)(struct intel_uc_fw *);
>  };
>  
>  void intel_uc_fw_init(struct drm_device *dev, struct intel_uc_fw *uc_fw,
>  	const char *uc_name, const char *fw_path);
>  int intel_uc_fw_check(struct drm_device *dev, struct intel_uc_fw *uc_fw);
>  void intel_uc_fw_fini(struct drm_device *dev, struct intel_uc_fw *uc_fw);
> +bool intel_uc_fw_version_check(struct intel_uc_fw *fw, const char *data);

This last function doesn't exist (and isn't needed).

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx