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