Re: [PATCH v2 6/6] drm/i915/guc: Move GuC declarations and functions into dedicated files

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Mon, 2017-10-02 at 14:01 +0000, Michal Wajdeczko wrote:
> We want to keep GuC specific code in separated files.
> 
> v2: move all functions in single patch (Joonas)
>     fix old checkpatch issues (Sagar)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> #1

<SNIP>

> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -0,0 +1,262 @@
> +/*
> + * Copyright © 2014-2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */

We do want to include "intel_guc.h" first and then other headers. After
all, all this effort is to get rid of i915_drv.h eventually so that
inter-file dependencies are called out more explicitly.

> +#include "i915_drv.h"
> +

<SNIP>

> +static
> +inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
> +{
> +	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);
> +
> +	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> +	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> +
> +	return offset;
> +}

Have these inline functions in their natural spots below. Them being
inline or not is bound to change.

> +
> +/* intel_guc.c */

This should be obvious, comment can be dropped.

> +void intel_guc_init_early(struct intel_guc *guc);
> +void intel_guc_init_send_regs(struct intel_guc *guc);
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> +int intel_guc_sample_forcewake(struct intel_guc *guc);
> +int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
> +int intel_guc_suspend(struct drm_i915_private *dev_priv);
> +int intel_guc_resume(struct drm_i915_private *dev_priv);
> +struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> +
> +/* intel_guc_loader.c */

These belong to intel_guc.c, so this comment can be dropped.

> +int intel_guc_select_fw(struct intel_guc *guc);
> +int intel_guc_init_hw(struct intel_guc *guc);

This goes under init_early at least, init_send_regs seems overly
specific and is not it like guc_init_mmio? Could be renamed later.

> +u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> +
> +/* i915_guc_submission.c */

A separate header for these.

> +int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> +int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> +void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +
> +/* intel_guc_log.c */

Ditto.

> +int intel_guc_log_create(struct intel_guc *guc);
> +void intel_guc_log_destroy(struct intel_guc *guc);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> +void i915_guc_log_register(struct drm_i915_private *dev_priv);
> +void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> +
> +#endif

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx