[Intel-gfx] [PATCH 1/4] drm/i915: Separate GuC/HuC specific functionality from intel_uc

[Kamble, Sagar A]

On 9/7/2017 5:54 PM, Michał Winiarski wrote:
> On Fri, Sep 01, 2017 at 11:02:09AM +0530, Sagar Arun Kamble wrote:
>> Removed unnecessary intel_uc.h includes as it is present in i915_drv.h.
>> Created intel_guc.c and intel_guc.h for placing GuC specific code.
>> Created intel_huc.h to refer to HuC specific functions.
>>
>> v2: Prepared intel_uc_common.h. huc_auth code declaration adjusted.
>> Moved enable/disable_communication to intel_uc.c (Michal)
> In v2 you also renamed things, moved things around (and addressed all of the
> other review comments from Michał).
Yes. Sorry. Will update in the next revision.
>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile              |   1 +
>>   drivers/gpu/drm/i915/i915_drv.c            |   1 -
>>   drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
>>   drivers/gpu/drm/i915/intel_guc.c           | 193 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc.h           | 200 +++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
>>   drivers/gpu/drm/i915/intel_huc.c           |  50 +-----
>>   drivers/gpu/drm/i915/intel_huc.h           |  38 +++++
>>   drivers/gpu/drm/i915/intel_uc.c            | 128 +--------------
>>   drivers/gpu/drm/i915/intel_uc.h            | 254 +----------------------------
>>   drivers/gpu/drm/i915/intel_uc_common.h     | 101 ++++++++++++
>>   11 files changed, 545 insertions(+), 423 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/intel_guc.c
>>   create mode 100644 drivers/gpu/drm/i915/intel_guc.h
>>   create mode 100644 drivers/gpu/drm/i915/intel_huc.h
>>   create mode 100644 drivers/gpu/drm/i915/intel_uc_common.h
> [SNIP]
>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>> index 22ae52b..c87a2b4 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -24,256 +24,8 @@
>>   #ifndef _INTEL_UC_H_
>>   #define _INTEL_UC_H_
>>   
> [SNIP]
>
>> -/* intel_huc.c */
>> -void intel_huc_select_fw(struct intel_huc *huc);
>> -void intel_huc_init_hw(struct intel_huc *huc);
>> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
>> +#include <intel_uc_common.h>
>> +#include <intel_guc.h>
>> +#include <intel_huc.h>
> ^^^
> Will this build? (well... it passed BAT, but it doesn't compile on my box).
> drivers/gpu/drm/i915 is not -I, so we should use quote marks, not angle
> brackets.
>
> Separate header, why? Can't we merge intel_uc_common.h with intel_uc.h?
>
> -Michał
Will change these to quote marks. I added this as separate header to not 
declare struct intel_uc_fw in same header as struct intel_guc and struct 
intel_huc.
We can merge but then it will take #include for intel_guc.h and 
intel_huc.h to the end of file.
If we want to keep intel_guc.h and intel_huc.h at the top, they should 
reference pointer to struct intel_uc_fw.
For readability I feel having intel_uc_common.h also seems intuitive.