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

Yu Dai
2015-04-28T15:19:36Z

On 04/28/2015 08:12 AM, Dave Gordon wrote:
> On 23/04/15 18:48, Dave Gordon wrote:
> > 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 ...
>
> Also, does intel_wopcm_init() meed to be called only once, during driver
> load? Or should it be called after a reset as well? Maybe it should be
> called from intel_guc_load_ucode() ? In which case it could live in one
> of the GuC-specific files and be static :)
>
> Possibly it belongs near the gem-guc-object allocator, since the setting
> of this register and the selection of GuC-accessible offsets within the
> GTT are related ...
>
>
In my local tree, this has been moved to right before load firmware.
This wopcm_init() was introduced for huc firmware loading. We must load
huc before guc. Since we don't have plan to upsteam huc frimware yet, as
you suggested, we can move this wopcm_init() to guc_load_ucode() make
code clean. In case we upstream huc firmware in future, we can put it
before huc loading.

Alex