Re: [Intel-gfx] [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader
On 07/17/2015 05:35 PM, O'Rourke, Tom wrote:
On Thu, Jul 09, 2015 at 07:29:05PM +0100, Dave Gordon wrote:
From: Alex Dai yu@intel.com
+static u32 get_core_family(struct drm_i915_private *dev_priv)
+{
+ switch (INTEL_INFO(dev_priv)-gen) {
+ case 8:
+ return GFXCORE_FAMILY_GEN8;
[TOR:] Should Gen 8 case be included here if only Gen 9 is supported?
Yes, we can remove this even Gen8 is capable but it is not supported by
these patch series anyway.
+
+
+ /* Set MMIO/WA for GuC init */
+ I915_WRITE(DRBMISC1, DOORBELL_ENABLE);
[TOR:] Should this DOORBELL_ENABLE be dropped? A note in
the BSpec indicates this is not needed, but also it should
be harmless.
Per response from firmware team / BSpec, we can remove this line.
+
+ /* Enable MIA caching. GuC clock gating is disabled. */
+ I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
[TOR:] Should guc clock gating be enabled? A note in the
BSpec indicates this should be disabled for certain
pre-production steppings; this note may not apply to later
steppings. Normally, the driver would enable guc clock
gating (bit 15, GUC_ENABLE_MIA_CLOCK_GATING).
There was a hang issue in GuC if clock gating is enabled. This has be
resolved for a while. We should enable this bit.
+
+ /* WaC6DisallowByGfxPause*/
+ I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
+
+ if (IS_SKYLAKE(dev))
+ I915_WRITE(GEN9_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
+ else
+ I915_WRITE(GEN8_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
[TOR:] Would a comment be helpful here? This line is correct
for Broxton (Gen 9 and not Skylake) but the constants are
reused from Gen 8.
+
BXT is Gen9 LP, which is using same mmio register as Gen8 for this case.
My suggestion:
s/GEN8_GT_DOORBELL_ENABLE/GT_DOORBELL_ENABLE/g
And, add definition below. Use it here to avoid confuse.
#define GEN9LP_GT_PM_CONFIG 0x138140
s/I915_WRITE(GEN8_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
/I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);/g
Thanks,
Alex
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader
On Thu, Jul 09, 2015 at 07:29:05PM +0100, Dave Gordon wrote:
From: Alex Dai yu@intel.com
This fetches the required firmware image from the filesystem,
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.
v2:
Various improvements per review comments by Chris Wilson
v3:
Removed 'wait' parameter to intel_guc_ucode_load() as firmware
prefetch is no longer supported in the common firmware loader,
per Daniel Vetter's request.
Firmware checker callback fn now returns errno rather than bool.
v4:
Squash uC-independent code into GuC-specifc loader [Daniel Vetter]
Don't keep the driver working (by falling back to execlist mode)
if GuC firmware loading fails [Daniel Vetter]
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 | 13 +
drivers/gpu/drm/i915/i915_reg.h | 4 +-
drivers/gpu/drm/i915/intel_guc.h| 67
drivers/gpu/drm/i915/intel_guc_loader.c | 536
7 files changed, 637 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 de21965..e604cfe 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -39,6 +39,9 @@ i915-y += i915_cmd_parser.o \
intel_ringbuffer.o \
intel_uncore.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 066c34c..958ab4f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -472,6 +472,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);
@@ -869,6 +870,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);
@@ -1120,6 +1123,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 4a512da..15b9202 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:
*/
@@ -1694,6 +1695,8 @@ struct drm_i915_private {
struct i915_virtual_gpu vgpu;
+ struct intel_guc guc;
+
struct intel_csr csr;
/* Display CSR-related protection */
@@ -1938,6 +1941,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__)++) \
@@ -2543,6 +2551,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 HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
INTEL_INFO(dev)-gen = 8)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dbbb649..e020309 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5074,6 +5074,19 @@ i915_gem_init_hw(struct drm_device *dev)
goto out;
}
+ /* We can't enable contexts until all firmware is loaded */
+ ret = intel_guc_ucode_load(dev);
+
+ /*
+ * If we got an error and GuC submission is enabled, map
+ * the error to -EIO so the GPU will be declared wedged.
+ * OTOH, if we didn't intend to use the GuC anyway, just
+ * discard the error and carry on.
+ */
+ ret =
Re: [Intel-gfx] [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader
On Thu, Jul 09, 2015 at 07:29:05PM +0100, Dave Gordon wrote: From: Alex Dai yu@intel.com This fetches the required firmware image from the filesystem, 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. v2: Various improvements per review comments by Chris Wilson v3: Removed 'wait' parameter to intel_guc_ucode_load() as firmware prefetch is no longer supported in the common firmware loader, per Daniel Vetter's request. Firmware checker callback fn now returns errno rather than bool. v4: Squash uC-independent code into GuC-specifc loader [Daniel Vetter] Don't keep the driver working (by falling back to execlist mode) if GuC firmware loading fails [Daniel Vetter] Issue: VIZ-4884 Signed-off-by: Alex Dai yu@intel.com Signed-off-by: Dave Gordon david.s.gor...@intel.com I think this is it, one comment below. diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index dbbb649..e020309 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5074,6 +5074,19 @@ i915_gem_init_hw(struct drm_device *dev) goto out; } + /* We can't enable contexts until all firmware is loaded */ + ret = intel_guc_ucode_load(dev); To stay in line with the current flow I think the request_firmware + create fw bo object code should be move into gem_init so that gem_init_hw is only responsible for loading the fw in the bo into hw. That will take care of not trying to re-request the firmware from userspace in resume/gpu reset code, which should simplify the status handling a lot. Also with just declaring the gpu wedged we could instead move the check below into the init_hw part of the guc load process. That would nicely encapsulate that decision and I'd expect take care of the other status codes - in the end callers really only care about -EIO or not here. But imo we can polish that after merging. All my other higher-level concerns with this have been addressed, so I think this is good to go in after detailed code review. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
