[Intel-gfx] [PATCH 05/15] drm/i915: GuC-specific firmware loader
Daniel Vetter
daniel at ffwll.ch
Mon Jul 6 11:12:29 PDT 2015
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.dai at 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.dai at intel.com>
> >>Signed-off-by: Dave Gordon <david.s.gordon at 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 my comments on
> >the irq routing code.
> >
> >Yes we can make this work, but givin our stellar track record with keeping
> >disabled features working it won't work for long. And it will impact us
> >with additional constraints until we give up and rip it out again. Not
> >worth it imo - if we decide to use the guc on a given platform we should
> >imo require it and stick to that decision for at least as long as the
> >driver is loaded. Developers can still change the option when reloading the
> >driver, users won't have a chance to cause trouble.
> >-Daniel
>
> Again, this isn't really "runtime" -- we're still in the driver loading
> stage here. This is analogous to the various "sanitize" functions where we
> cross-check what options have been set and decide which to override, except
> that here we can't determine whether we're going to respect the default or
> user-specified request for GuC submission mode until we know whether we have
> valid firmware for the GuC.
The problem is that a pile of code has run already (specifically irq
setup). Not a problem from the correctness pov it can all be made to work,
but from a longer-term maintainance pov. It'll just bitrot until it's
ripped out. But until that happens everyone has to keep it in mind and
try not to break this fallback. Too much trouble imo for no real benefit.
> At this point, we haven't submitted any batches, so the main point of use of
> this flag -- in the submission path, to switch between execlist and GuC
> modes -- has never yet been executed. So there should be no problem with
> changing the value before it's first used.
>
> And this is a one-way switch; you (or the default config) asked for GuC
> submission, but we can't support it so we disable the option. There's no way
> to switch it back on without reloading the driver. So this /is/ the point at
> which we decide to use the GuC on a given platform and then stick to that
> decision for at least as long as the driver is loaded.
>
> We have to support execlist mode for the foreseeable future anyway, so using
> it on a machine which (we think) ought to be GuC-capable doesn't add /any/
> extra maintenance overhead at all.
>
> Why break the user's machine unnecessarily? With real "end-users",
> especially those who have never used Linux before, you only get one chance.
> Sometimes I've installed Linux on a (Windows-using) friend's machine, and it
> hasn't worked first time. Then I switch to another VT, type some magic
> incantations, and 10 minutes later we have a usable login screen. Will they
> adopt Linux? Unlikely :( No matter how good it looks thereafter, if the
> machine's hardware doesn't work with the distro straight out of the box,
> they're just not going to believe it's something they can use. So it's very
> important that everything essential to the first-time experience works even
> when misconfigured -- and nothing is more essential than the display driver
> (networking and wi-fi are the next things that will put the user off if they
> don't work -- and they're also drivers that commonly rely on firmware
> blobs).
Those end-users install fedora or ubuntu which get the firmware blob
loading just right. None of the big other drivers bother with falling back
to some other mode if the firmware they need isn't there: radeon just
outright bails out, nouveau just disables accel or runtime pm if the
firmware blob is only needed for these features.
The only user I can see are people allergic to firmware blobs (they can
use execlist and we don't care) or developers (can change mod options
too). Misconfigured systems from newbies is not a target market, neither
for intel nor for upstream (yes we WONTFIX bugs of people with funky
configs if the problem doesn't exist on a properly configured distro).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list