[Intel-gfx] [PATCH v3 9/9] drm/i915/guc: Move GuC core definitions into dedicated files

[Michal Wajdeczko]

On Wed, 04 Oct 2017 19:17:21 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-03 17:36:07)
>> We want to keep GuC specific code in separated files.
>>
>> v2: move all functions in single patch (Joonas)
>>     fix old checkpatch issues (Sagar)
>>
>> v3: rebased
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com> #1
>> ---

<snip>

>> +#ifndef _INTEL_GUC_H_
>> +#define _INTEL_GUC_H_
>> +
>> +#include "intel_uncore.h"
>> +#include "intel_guc_fwif.h"
>> +#include "intel_guc_ct.h"
>> +#include "intel_guc_log.h"
>> +#include "intel_uc_fw.h"
>> +#include "i915_guc_reg.h"
>> +#include "i915_vma.h"
>
> Hmm. Are they required for the header? Or for the C body?

For the header.

#include "intel_uncore.h" --> enum forcewake_domains
#include "intel_guc_fwif.h" --> GUC_NUM_DOORBELLS
#include "intel_guc_ct.h" --> struct intel_guc_ct
#include "intel_guc_log.h" --> struct intel_guc_log
#include "intel_uc_fw.h" --> struct intel_uc_fw
#include "i915_guc_reg.h" --> GUC_WOPCM_TOP
#include "i915_vma.h" --> i915_ggtt_offset

>
>> +
>> +struct intel_guc {
>> +       struct intel_uc_fw fw;
>> +       struct intel_guc_log log;
>> +       struct intel_guc_ct ct;
>
> Most of the embedded types ^^ will not require all of these headers.

Their requirements are handled in their own .h

>
>> +
>> +       /* Log snapshot if GuC errors during load */
>> +       struct drm_i915_gem_object *load_err_log;
>> +
>> +       /* intel_guc_recv interrupt related state */
>> +       bool interrupts_enabled;
>> +
>> +       struct i915_vma *ads_vma;
>> +       struct i915_vma *stage_desc_pool;
>> +       void *stage_desc_pool_vaddr;
>> +       struct ida stage_ids;
>> +
>> +       struct i915_guc_client *execbuf_client;
>> +
>> +       DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
>
> But we do need <linux/bitmap.h>

No, only <linux/types.h>

https://cgit.freedesktop.org/drm-tip/tree/include/linux/types.h#n9

>
>> +       uint32_t db_cacheline;          /* Cyclic counter mod pagesize   
>> */
>
> Too soon to s/uint32_t/u32/ ?

Yes.

>
>> +
>> +       /* GuC's FW specific registers used in MMIO send */
>> +       struct {
>> +               u32 base;
>> +               unsigned int count;
>> +               enum forcewake_domains fw_domains;
>> +       } send_regs;
>> +
>> +       /* To serialize the intel_guc_send actions */
>> +       struct mutex send_mutex;
>> +
>> +       /* GuC's FW specific send function */
>> +       int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
>> +
>> +       /* GuC's FW specific notify function */
>> +       void (*notify)(struct intel_guc *guc);
>> +};
>> +
>> +static
>> +inline int intel_guc_send(struct intel_guc *guc, const u32 *action,  
>> u32 len)
>
> Unusual line splitting.

Will fix.

>
>> +{
>> +       return guc->send(guc, action, len);
>> +}
>> +
>> +static inline void intel_guc_notify(struct intel_guc *guc)
>> +{
>> +       guc->notify(guc);
>> +}
>> +
>> +static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>> +{
>> +       u32 offset = i915_ggtt_offset(vma);
>
> So here we required i915_vma.h, ok.
>
> It's pretty much a carbon copy, so
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>
> However, there's still plenty to polish.
> -Chris

Thanks,
Michal