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

[Yu Dai <yu.dai@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx