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

2015-07-06 Thread Dave Gordon 

On 06/07/15 15:28, Daniel Vetter wrote:

On Fri, Jul 03, 2015 at 01:30:27PM +0100, Dave Gordon wrote:

From: Alex Dai yu@intel.com

This uses the common firmware loader to fetch the firmware image,
then loads it into the GuC's memory via a dedicated DMA engine.

This patch is derived from GuC loading work originally done by
Vinit Azad and Ben Widawsky. It has been reconstructed to accord
with the common firmware loading mechanism by Dave Gordon as well
as new firmware layout etc.

v2:
 Various improvements per review comments by Chris Wilson

v3:
 Removed 'wait' parameter to intel_guc_ucode_load() as prefetch
 is no longer supported in the common firmware loader, per
Daniel Vetter's request.
 F/w checker callback fn now returns errno rather than bool.

Issue: VIZ-4884
Signed-off-by: Alex Dai yu@intel.com
Signed-off-by: Dave Gordon david.s.gor...@intel.com
---
  drivers/gpu/drm/i915/Makefile   |   3 +
  drivers/gpu/drm/i915/i915_dma.c |   4 +
  drivers/gpu/drm/i915/i915_drv.h |  11 +
  drivers/gpu/drm/i915/i915_gem.c |   8 +
  drivers/gpu/drm/i915/i915_reg.h |   4 +-
  drivers/gpu/drm/i915/intel_guc.h|  49 
  drivers/gpu/drm/i915/intel_guc_loader.c | 448 
  7 files changed, 526 insertions(+), 1 deletion(-)
  create mode 100644 drivers/gpu/drm/i915/intel_guc.h
  create mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f1f80fc..62a8c83 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -42,6 +42,9 @@ i915-y += i915_cmd_parser.o \
  # generic ancilliary microcontroller support
  i915-y += intel_uc_loader.o

+# general-purpose microcontroller (GuC) support
+i915-y += intel_guc_loader.o
+
  # autogenerated null render state
  i915-y += intel_renderstate_gen6.o \
  intel_renderstate_gen7.o \
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..730d91b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -469,6 +469,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);
@@ -866,6 +867,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long 
flags)

intel_uncore_init(dev);

+   intel_guc_ucode_init(dev);
+
/* Load CSR Firmware for SKL */
intel_csr_ucode_init(dev);

@@ -1117,6 +1120,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 9618f57..a7ccac5 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:
   */
@@ -1687,6 +1688,8 @@ struct drm_i915_private {

struct i915_virtual_gpu vgpu;

+   struct intel_guc guc;
+
struct intel_csr csr;

/* Display CSR-related protection */
@@ -1931,6 +1934,11 @@ static inline struct drm_i915_private 
*dev_to_i915(struct device *dev)
return to_i915(dev_get_drvdata(dev));
  }

+static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
+{
+   return container_of(guc, struct drm_i915_private, guc);
+}
+
  /* Iterate over initialised rings */
  #define for_each_ring(ring__, dev_priv__, i__) \
for ((i__) = 0; (i__)  I915_NUM_RINGS; (i__)++) \
@@ -2539,6 +2547,9 @@ struct drm_i915_cmd_table {

  #define HAS_CSR(dev)  (IS_SKYLAKE(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
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index aa8f4c3..80d7890 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5076,6 +5076,14 @@ i915_gem_init_hw(struct drm_device *dev)
goto out;
}

+   /*
+* We can't enable contexts until all firmware is loaded; if this
+* fails, disable GuC submissions and fall back to execlist mode
+*/
+   ret = intel_guc_ucode_load(dev);
+   if (ret)
+   i915.enable_guc_submission = false;


I want an -EIO or similar here since runtime fallbacks to other modes
really aren't great from a maintainance perspective, see

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

2015-07-06 Thread Daniel Vetter 
On Mon, Jul 06, 2015 at 05:37:57PM +0100, Dave Gordon wrote:
 On 06/07/15 15:28, Daniel Vetter wrote:
 On Fri, Jul 03, 2015 at 01:30:27PM +0100, Dave Gordon wrote:
 From: Alex Dai yu@intel.com
 
 This uses the common firmware loader to fetch the firmware image,
 then loads it into the GuC's memory via a dedicated DMA engine.
 
 This patch is derived from GuC loading work originally done by
 Vinit Azad and Ben Widawsky. It has been reconstructed to accord
 with the common firmware loading mechanism by Dave Gordon as well
 as new firmware layout etc.
 
 v2:
  Various improvements per review comments by Chris Wilson
 
 v3:
  Removed 'wait' parameter to intel_guc_ucode_load() as prefetch
  is no longer supported in the common firmware loader, per
 Daniel Vetter's request.
  F/w checker callback fn now returns errno rather than bool.
 
 Issue: VIZ-4884
 Signed-off-by: Alex Dai yu@intel.com
 Signed-off-by: Dave Gordon david.s.gor...@intel.com
 ---
   drivers/gpu/drm/i915/Makefile   |   3 +
   drivers/gpu/drm/i915/i915_dma.c |   4 +
   drivers/gpu/drm/i915/i915_drv.h |  11 +
   drivers/gpu/drm/i915/i915_gem.c |   8 +
   drivers/gpu/drm/i915/i915_reg.h |   4 +-
   drivers/gpu/drm/i915/intel_guc.h|  49 
   drivers/gpu/drm/i915/intel_guc_loader.c | 448 
  
   7 files changed, 526 insertions(+), 1 deletion(-)
   create mode 100644 drivers/gpu/drm/i915/intel_guc.h
   create mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c
 
 diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
 index f1f80fc..62a8c83 100644
 --- a/drivers/gpu/drm/i915/Makefile
 +++ b/drivers/gpu/drm/i915/Makefile
 @@ -42,6 +42,9 @@ i915-y += i915_cmd_parser.o \
   # generic ancilliary microcontroller support
   i915-y += intel_uc_loader.o
 
 +# general-purpose microcontroller (GuC) support
 +i915-y += intel_guc_loader.o
 +
   # autogenerated null render state
   i915-y += intel_renderstate_gen6.o \
   intel_renderstate_gen7.o \
 diff --git a/drivers/gpu/drm/i915/i915_dma.c 
 b/drivers/gpu/drm/i915/i915_dma.c
 index c5349fa..730d91b 100644
 --- a/drivers/gpu/drm/i915/i915_dma.c
 +++ b/drivers/gpu/drm/i915/i915_dma.c
 @@ -469,6 +469,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);
 @@ -866,6 +867,8 @@ int i915_driver_load(struct drm_device *dev, unsigned 
 long flags)
 
 intel_uncore_init(dev);
 
 +   intel_guc_ucode_init(dev);
 +
 /* Load CSR Firmware for SKL */
 intel_csr_ucode_init(dev);
 
 @@ -1117,6 +1120,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 9618f57..a7ccac5 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:
*/
 @@ -1687,6 +1688,8 @@ struct drm_i915_private {
 
 struct i915_virtual_gpu vgpu;
 
 +   struct intel_guc guc;
 +
 struct intel_csr csr;
 
 /* Display CSR-related protection */
 @@ -1931,6 +1934,11 @@ static inline struct drm_i915_private 
 *dev_to_i915(struct device *dev)
 return to_i915(dev_get_drvdata(dev));
   }
 
 +static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 +{
 +   return container_of(guc, struct drm_i915_private, guc);
 +}
 +
   /* Iterate over initialised rings */
   #define for_each_ring(ring__, dev_priv__, i__) \
 for ((i__) = 0; (i__)  I915_NUM_RINGS; (i__)++) \
 @@ -2539,6 +2547,9 @@ struct drm_i915_cmd_table {
 
   #define HAS_CSR(dev)  (IS_SKYLAKE(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
 diff --git a/drivers/gpu/drm/i915/i915_gem.c 
 b/drivers/gpu/drm/i915/i915_gem.c
 index aa8f4c3..80d7890 100644
 --- a/drivers/gpu/drm/i915/i915_gem.c
 +++ b/drivers/gpu/drm/i915/i915_gem.c
 @@ -5076,6 +5076,14 @@ i915_gem_init_hw(struct drm_device *dev)
 goto out;
 }
 
 +   /*
 +* We can't enable contexts until all firmware is loaded; if this
 +* fails, disable GuC submissions and fall back to execlist mode
 +*/
 +   ret = intel_guc_ucode_load(dev);
 +   if (ret)
 +   i915.enable_guc_submission = false;
 
 I want an -EIO or similar here since runtime

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

2015-07-06 Thread Daniel Vetter 
On Fri, Jul 03, 2015 at 01:30:27PM +0100, Dave Gordon wrote:
 From: Alex Dai yu@intel.com
 
 This uses the common firmware loader to fetch the firmware image,
 then loads it into the GuC's memory via a dedicated DMA engine.
 
 This patch is derived from GuC loading work originally done by
 Vinit Azad and Ben Widawsky. It has been reconstructed to accord
 with the common firmware loading mechanism by Dave Gordon as well
 as new firmware layout etc.
 
 v2:
 Various improvements per review comments by Chris Wilson
 
 v3:
 Removed 'wait' parameter to intel_guc_ucode_load() as prefetch
 is no longer supported in the common firmware loader, per
   Daniel Vetter's request.
 F/w checker callback fn now returns errno rather than bool.
 
 Issue: VIZ-4884
 Signed-off-by: Alex Dai yu@intel.com
 Signed-off-by: Dave Gordon david.s.gor...@intel.com
 ---
  drivers/gpu/drm/i915/Makefile   |   3 +
  drivers/gpu/drm/i915/i915_dma.c |   4 +
  drivers/gpu/drm/i915/i915_drv.h |  11 +
  drivers/gpu/drm/i915/i915_gem.c |   8 +
  drivers/gpu/drm/i915/i915_reg.h |   4 +-
  drivers/gpu/drm/i915/intel_guc.h|  49 
  drivers/gpu/drm/i915/intel_guc_loader.c | 448 
 
  7 files changed, 526 insertions(+), 1 deletion(-)
  create mode 100644 drivers/gpu/drm/i915/intel_guc.h
  create mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c
 
 diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
 index f1f80fc..62a8c83 100644
 --- a/drivers/gpu/drm/i915/Makefile
 +++ b/drivers/gpu/drm/i915/Makefile
 @@ -42,6 +42,9 @@ i915-y += i915_cmd_parser.o \
  # generic ancilliary microcontroller support
  i915-y += intel_uc_loader.o
  
 +# general-purpose microcontroller (GuC) support
 +i915-y += intel_guc_loader.o
 +
  # autogenerated null render state
  i915-y += intel_renderstate_gen6.o \
 intel_renderstate_gen7.o \
 diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
 index c5349fa..730d91b 100644
 --- a/drivers/gpu/drm/i915/i915_dma.c
 +++ b/drivers/gpu/drm/i915/i915_dma.c
 @@ -469,6 +469,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);
 @@ -866,6 +867,8 @@ int i915_driver_load(struct drm_device *dev, unsigned 
 long flags)
  
   intel_uncore_init(dev);
  
 + intel_guc_ucode_init(dev);
 +
   /* Load CSR Firmware for SKL */
   intel_csr_ucode_init(dev);
  
 @@ -1117,6 +1120,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 9618f57..a7ccac5 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:
   */
 @@ -1687,6 +1688,8 @@ struct drm_i915_private {
  
   struct i915_virtual_gpu vgpu;
  
 + struct intel_guc guc;
 +
   struct intel_csr csr;
  
   /* Display CSR-related protection */
 @@ -1931,6 +1934,11 @@ static inline struct drm_i915_private 
 *dev_to_i915(struct device *dev)
   return to_i915(dev_get_drvdata(dev));
  }
  
 +static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 +{
 + return container_of(guc, struct drm_i915_private, guc);
 +}
 +
  /* Iterate over initialised rings */
  #define for_each_ring(ring__, dev_priv__, i__) \
   for ((i__) = 0; (i__)  I915_NUM_RINGS; (i__)++) \
 @@ -2539,6 +2547,9 @@ struct drm_i915_cmd_table {
  
  #define HAS_CSR(dev) (IS_SKYLAKE(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
 diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
 index aa8f4c3..80d7890 100644
 --- a/drivers/gpu/drm/i915/i915_gem.c
 +++ b/drivers/gpu/drm/i915/i915_gem.c
 @@ -5076,6 +5076,14 @@ i915_gem_init_hw(struct drm_device *dev)
   goto out;
   }
  
 + /*
 +  * We can't enable contexts until all firmware is loaded; if this
 +  * fails, disable GuC submissions and fall back to execlist mode
 +  */
 + ret = intel_guc_ucode_load(dev);
 + if (ret)
 + i915.enable_guc_submission = false;

I want an -EIO or similar here since runtime fallbacks to other modes
really aren't great from a maintainance perspective,

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

2015-06-19 Thread Dave Gordon 
On 18/06/15 21:12, Chris Wilson wrote:
 On Thu, Jun 18, 2015 at 10:53:10AM -0700, Yu Dai wrote:


 On 06/15/2015 01:30 PM, Chris Wilson wrote:
 On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
 +  /* 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.

This particular object wasn't allocated with that function; that's only
used for objects that need to be permanently accessible by the GuC
(context pool, GuC logbuffer, per-client structure). As I already
mentioned in another reply, /this/ one was pinned (and will be unpinned)
by the *immediate caller* of this function.

.Dave.

 The basic rules when reviewing is pinning is:
 - is there a reason for this pin?
 - is the lifetime of the pin bound to the hardware access?
 - are the pad-to-size/alignment correct?
 - is the vma in the wrong location?
 
 Pinning early (and then not even stating in the function preamble that
 you expect the object to be pinned) makes it hard to review both the
 reason and check the lifetime. An easy solution to avoiding the
 assumption of having a pinned object is to pass around the vma instead.
 Though because you pin too early it is not clear the reason for the pin
 nor that you only pin it for the lifetime of the hardware access, and
 you have to scour the code to ensure that the pin isn't randomly dropped
 or reused for another access.
 -Chris

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

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

2015-06-18 Thread Yu Dai 



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)  0x);
 +
 +  /* 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

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

2015-06-18 Thread Dave Gordon 
On 15/06/15 21:30, Chris Wilson wrote:
 On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
 +/* We can't enable contexts until all firmware is loaded */
 +ret = intel_guc_ucode_load(dev, false);
 
 Pardon. I know context initialisation is broken, but adding to that
 breakage is not pleasant.

Sorry, but that's just the way it works. If you want to use the GuC for
batch submission, then you cannot submit any commands to any engine via
the GuC before its firmware is loaded, nor can you submit anything at
all directly to the ELSPs.

However in /this/ patch the 'false' above should have been 'true' to
give synchronous load semantics; and then ignoring the return is
intentional, because either it's worked and we're going to use the GuC,
or it hasn't and we're not (and it's already printed a message). Then
there's a later patch that tries to decouple engine MMIO setup from
engine setup using batches  contexts, at which point we can make use of
the return code.

  ret = i915_gem_context_enable(dev_priv);
  if (ret  ret != -EIO) {
  DRM_ERROR(Context enable failed %d\n, ret);
 
 diff --git a/drivers/gpu/drm/i915/intel_guc.h 
 b/drivers/gpu/drm/i915/intel_guc.h
 index 82367c9..0b44265 100644
 --- a/drivers/gpu/drm/i915/intel_guc.h
 +++ b/drivers/gpu/drm/i915/intel_guc.h
 @@ -166,4 +166,9 @@ struct intel_guc {
  #define GUC_WD_VECS_IER 0xC558
  #define GUC_PM_P24C_IER 0xC55C
  
 +/* intel_guc_loader.c */
 +extern void intel_guc_ucode_init(struct drm_device *dev);
 +extern int intel_guc_ucode_load(struct drm_device *dev, bool wait);
 +extern void intel_guc_ucode_fini(struct drm_device *dev);
 +
  #endif
 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 000..16eef4c
 --- /dev/null
 +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
 @@ -0,0 +1,416 @@
 +/*
 + * Copyright © 2014 Intel Corporation
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a
 + * copy of this software and associated documentation files (the 
 Software),
 + * to deal in the Software without restriction, including without limitation
 + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
 + * and/or sell copies of the Software, and to permit persons to whom the
 + * Software is furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice (including the next
 + * paragraph) shall be included in all copies or substantial portions of the
 + * Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS 
 OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
 + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
 OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
 DEALINGS
 + * IN THE SOFTWARE.
 + *
 + * Authors:
 + *Vinit Azad vinit.a...@intel.com
 + *Ben Widawsky b...@bwidawsk.net
 + *Dave Gordon david.s.gor...@intel.com
 + *Alex Dai yu@intel.com
 + */
 +#include linux/firmware.h
 +#include i915_drv.h
 +#include intel_guc.h
 +
 +/**
 + * DOC: GuC
 + *
 + * intel_guc:
 + * Top level structure of guc. It handles firmware loading and manages 
 client
 + * pool and doorbells. intel_guc owns a i915_guc_client to replace the 
 legacy
 + * ExecList submission.
 + *
 + * Firmware versioning:
 + * The firmware build process will generate a version header file with 
 major and
 + * minor version defined. The versions are built into CSS header of 
 firmware.
 + * i915 kernel driver set the minimal firmware version required per 
 platform.
 + * The firmware installation package will install (symbolic link) proper 
 version
 + * of firmware.
 + *
 + * GuC address space:
 + * GuC does not allow any gfx GGTT address that falls into range [0, 
 WOPCM_TOP),
 + * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top 
 address is
 + * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
 + * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
 + *
 + * Firmware log:
 + * Firmware log is enabled by setting i915.guc_log_level to non-negative 
 level.
 + * Log data is printed out via reading debugfs i915_guc_log_dump. Reading 
 from
 + * i915_guc_load_status will print out firmware loading status and scratch
 + * registers value.
 + *
 + */
 +
 +#define I915_SKL_GUC_UCODE i915/skl_guc_ver3.bin
 +MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
 +
 +static u32 get_gttype(struct drm_device *dev)
 +{
 +/* XXX: GT type based on PCI device ID? field seems unused by fw */
 +return 0;
 +}
 +
 +static u32 get_core_family(struct drm_device *dev)
 
 For new code we really should be in the habit of passing around the

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

2015-06-18 Thread Chris Wilson 
On Thu, Jun 18, 2015 at 10:53:10AM -0700, Yu Dai wrote:
 
 
 On 06/15/2015 01:30 PM, Chris Wilson wrote:
 On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
  +  /* 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.

The basic rules when reviewing is pinning is:
- is there a reason for this pin?
- is the lifetime of the pin bound to the hardware access?
- are the pad-to-size/alignment correct?
- is the vma in the wrong location?

Pinning early (and then not even stating in the function preamble that
you expect the object to be pinned) makes it hard to review both the
reason and check the lifetime. An easy solution to avoiding the
assumption of having a pinned object is to pass around the vma instead.
Though because you pin too early it is not clear the reason for the pin
nor that you only pin it for the lifetime of the hardware access, and
you have to scour the code to ensure that the pin isn't randomly dropped
or reused for another access.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2015-06-15 Thread Chris Wilson 
On Mon, Jun 15, 2015 at 07:36:23PM +0100, Dave Gordon wrote:
 + /* We can't enable contexts until all firmware is loaded */
 + ret = intel_guc_ucode_load(dev, false);

Pardon. I know context initialisation is broken, but adding to that
breakage is not pleasant.

   ret = i915_gem_context_enable(dev_priv);
   if (ret  ret != -EIO) {
   DRM_ERROR(Context enable failed %d\n, ret);

 diff --git a/drivers/gpu/drm/i915/intel_guc.h 
 b/drivers/gpu/drm/i915/intel_guc.h
 index 82367c9..0b44265 100644
 --- a/drivers/gpu/drm/i915/intel_guc.h
 +++ b/drivers/gpu/drm/i915/intel_guc.h
 @@ -166,4 +166,9 @@ struct intel_guc {
  #define GUC_WD_VECS_IER  0xC558
  #define GUC_PM_P24C_IER  0xC55C
  
 +/* intel_guc_loader.c */
 +extern void intel_guc_ucode_init(struct drm_device *dev);
 +extern int intel_guc_ucode_load(struct drm_device *dev, bool wait);
 +extern void intel_guc_ucode_fini(struct drm_device *dev);
 +
  #endif
 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 000..16eef4c
 --- /dev/null
 +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
 @@ -0,0 +1,416 @@
 +/*
 + * Copyright © 2014 Intel Corporation
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a
 + * copy of this software and associated documentation files (the Software),
 + * to deal in the Software without restriction, including without limitation
 + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
 + * and/or sell copies of the Software, and to permit persons to whom the
 + * Software is furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice (including the next
 + * paragraph) shall be included in all copies or substantial portions of the
 + * Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
 + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
 DEALINGS
 + * IN THE SOFTWARE.
 + *
 + * Authors:
 + *Vinit Azad vinit.a...@intel.com
 + *Ben Widawsky b...@bwidawsk.net
 + *Dave Gordon david.s.gor...@intel.com
 + *Alex Dai yu@intel.com
 + */
 +#include linux/firmware.h
 +#include i915_drv.h
 +#include intel_guc.h
 +
 +/**
 + * DOC: GuC
 + *
 + * intel_guc:
 + * Top level structure of guc. It handles firmware loading and manages client
 + * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
 + * ExecList submission.
 + *
 + * Firmware versioning:
 + * The firmware build process will generate a version header file with major 
 and
 + * minor version defined. The versions are built into CSS header of firmware.
 + * i915 kernel driver set the minimal firmware version required per platform.
 + * The firmware installation package will install (symbolic link) proper 
 version
 + * of firmware.
 + *
 + * GuC address space:
 + * GuC does not allow any gfx GGTT address that falls into range [0, 
 WOPCM_TOP),
 + * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top 
 address is
 + * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
 + * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
 + *
 + * Firmware log:
 + * Firmware log is enabled by setting i915.guc_log_level to non-negative 
 level.
 + * Log data is printed out via reading debugfs i915_guc_log_dump. Reading 
 from
 + * i915_guc_load_status will print out firmware loading status and scratch
 + * registers value.
 + *
 + */
 +
 +#define I915_SKL_GUC_UCODE i915/skl_guc_ver3.bin
 +MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
 +
 +static u32 get_gttype(struct drm_device *dev)
 +{
 + /* XXX: GT type based on PCI device ID? field seems unused by fw */
 + return 0;
 +}
 +
 +static u32 get_core_family(struct drm_device *dev)

For new code we really should be in the habit of passing around the
right pointer, not dev.

 +{
 + switch (INTEL_INFO(dev)-gen) {
 + case 8:
 + return GFXCORE_FAMILY_GEN8;
 + case 9:
 + return GFXCORE_FAMILY_GEN9;
 + default:
 + DRM_ERROR(GUC: unknown gen for scheduler init\n);
 + return GFXCORE_FAMILY_FORCE_ULONG;
 + }
 +}
 +
 +static void set_guc_init_params(struct drm_i915_private *dev_priv)
 +{
 + struct intel_guc *guc = dev_priv-guc;
 + u32 params[GUC_CTL_MAX_DWORDS];
 + int i;
 +
 + memset(params, 0, sizeof(params));
 +
 + params[GUC_CTL_DEVICE_INFO] |=
 + (get_gttype(dev_priv-dev)  GUC_CTL_GTTYPE_SHIFT) |
 + (get_core_family(dev_priv-dev)  GUC_CTL_COREFAMILY_SHIFT);