[Intel-gfx] [PATCH v3 04/15] drm/i915: GuC firmware loader

[Dave Gordon]

On 23/04/15 18:48, Dave Gordon wrote:
> On 17/04/15 22:21, yu.dai at intel.com wrote:
>> From: Alex Dai <yu.dai at 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 at 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 ...

.Dave.