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

Michal Wajdeczko michal.wajdeczko at intel.com
Wed Aug 9 13:58:44 UTC 2017


On Wed, Aug 09, 2017 at 03:53:45PM +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.
> 
> 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              |   2 +
>  drivers/gpu/drm/i915/i915_drv.c            |   1 -
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +
>  drivers/gpu/drm/i915/i915_guc_submission.c |   1 -
>  drivers/gpu/drm/i915/intel_guc.c           | 184 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h           | 203 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   1 -
>  drivers/gpu/drm/i915/intel_huc.c           |   2 -
>  drivers/gpu/drm/i915/intel_huc.h           |  38 ++++++
>  drivers/gpu/drm/i915/intel_uc.c            | 159 +---------------------
>  drivers/gpu/drm/i915/intel_uc.h            | 178 -------------------------
>  11 files changed, 430 insertions(+), 341 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
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f822731..efa7605 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -58,6 +58,8 @@ i915-y += i915_cmd_parser.o \
>  
>  # general-purpose microcontroller (GuC) support
>  i915-y += intel_uc.o \
> +	  intel_guc.o \
> +	  intel_huc.o \

Please leave intel_huc.o below, *after* all intel_guc... files

>  	  intel_guc_ct.o \
>  	  intel_guc_log.o \
>  	  intel_guc_loader.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 214555e..5512cce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -50,7 +50,6 @@
>  #include "i915_trace.h"
>  #include "i915_vgpu.h"
>  #include "intel_drv.h"
> -#include "intel_uc.h"
>  
>  static struct drm_driver driver;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2c7456f..085647a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -59,6 +59,8 @@
>  #include "intel_bios.h"
>  #include "intel_dpll_mgr.h"
>  #include "intel_uc.h"
> +#include "intel_guc.h"
> +#include "intel_huc.h"

Hmm, I'm not sure this is right direction.
We should use intel_uc.h as super set of all our [gh]uc components.

>  #include "intel_lrc.h"
>  #include "intel_ringbuffer.h"
>  
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e93..602ae8a 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -23,7 +23,6 @@
>   */
>  #include <linux/circ_buf.h>
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  #include <trace/events/dma_fence.h>
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> new file mode 100644
> index 0000000..a812d3d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -0,0 +1,184 @@
> +/*
> + * Copyright © 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.
> + *
> + */
> +
> +#include "i915_drv.h"
> +
> +inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)

Why "static" was dropped ?
Also, please keep this function near other "send" functions

> +{
> +	GEM_BUG_ON(!guc->send_regs.base);
> +	GEM_BUG_ON(!guc->send_regs.count);
> +	GEM_BUG_ON(i >= guc->send_regs.count);
> +
> +	return _MMIO(guc->send_regs.base + 4 * i);
> +}
> +
> +void guc_capture_load_err_log(struct intel_guc *guc)
> +{
> +	if (!guc->log.vma || i915.guc_log_level < 0)
> +		return;
> +
> +	if (!guc->load_err_log)
> +		guc->load_err_log = i915_gem_object_get(guc->log.vma->obj);
> +}
> +
> +void guc_free_load_err_log(struct intel_guc *guc)
> +{
> +	if (guc->load_err_log)
> +		i915_gem_object_put(guc->load_err_log);
> +}

I guess above two functions can be treated as releated to "uc" activity and
as such can stay in intel_uc.c

> +
> +static void guc_init_send_regs(struct intel_guc *guc)

Maybe this function shall be named as "intel_guc_init()" and called only
once directly from intel_uc_init_hw() ?

> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	enum forcewake_domains fw_domains = 0;
> +	unsigned int i;
> +
> +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +
> +	for (i = 0; i < guc->send_regs.count; i++) {
> +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> +					guc_send_reg(guc, i),
> +					FW_REG_READ | FW_REG_WRITE);
> +	}
> +	guc->send_regs.fw_domains = fw_domains;
> +}
> +
> +int guc_enable_communication(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	guc_init_send_regs(guc);
> +
> +	if (HAS_GUC_CT(dev_priv))
> +		return intel_guc_enable_ct(guc);
> +
> +	guc->send = intel_guc_send_mmio;
> +	return 0;
> +}
> +
> +void guc_disable_communication(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	if (HAS_GUC_CT(dev_priv))
> +		intel_guc_disable_ct(guc);
> +
> +	guc->send = intel_guc_send_nop;
> +}

I've mixed feelings about moving [enable|disable] functions here...

> +
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> +{
> +	WARN(1, "Unexpected send: action=%#x\n", *action);
> +	return -ENODEV;
> +}
> +
> +/*
> + * This function implements the MMIO based host to GuC interface.
> + */
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 status;
> +	int i;
> +	int ret;
> +
> +	GEM_BUG_ON(!len);
> +	GEM_BUG_ON(len > guc->send_regs.count);
> +
> +	/* If CT is available, we expect to use MMIO only during init/fini */
> +	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
> +		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> +		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> +
> +	mutex_lock(&guc->send_mutex);
> +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> +
> +	for (i = 0; i < len; i++)
> +		I915_WRITE(guc_send_reg(guc, i), action[i]);
> +
> +	POSTING_READ(guc_send_reg(guc, i - 1));
> +
> +	intel_guc_notify(guc);
> +
> +	/*
> +	 * No GuC command should ever take longer than 10ms.
> +	 * Fast commands should still complete in 10us.
> +	 */
> +	ret = __intel_wait_for_register_fw(dev_priv,
> +					   guc_send_reg(guc, 0),
> +					   INTEL_GUC_RECV_MASK,
> +					   INTEL_GUC_RECV_MASK,
> +					   10, 10, &status);
> +	if (status != INTEL_GUC_STATUS_SUCCESS) {
> +		/*
> +		 * Either the GuC explicitly returned an error (which
> +		 * we convert to -EIO here) or no response at all was
> +		 * received within the timeout limit (-ETIMEDOUT)
> +		 */
> +		if (ret != -ETIMEDOUT)
> +			ret = -EIO;
> +
> +		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
> +			 " ret=%d status=0x%08X response=0x%08X\n",
> +			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
> +	}
> +
> +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> +	mutex_unlock(&guc->send_mutex);
> +
> +	return ret;
> +}
> +
> +int intel_guc_sample_forcewake(struct intel_guc *guc)

This function can be moved to the end.

> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 action[2];
> +
> +	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
> +	/* WaRsDisableCoarsePowerGating:skl,bxt */
> +	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> +		action[1] = 0;
> +	else
> +		/* bit 0 and 1 are for Render and Media domain separately */
> +		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
> +
> +	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> +}
> +
> +static void guc_write_irq_trigger(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> +}
> +
> +void intel_guc_init_early(struct intel_guc *guc)

Please move this function as far to the top as possible.

> +{
> +	intel_guc_ct_init_early(&guc->ct);
> +
> +	mutex_init(&guc->send_mutex);
> +	guc->send = intel_guc_send_nop;
> +	guc->notify = guc_write_irq_trigger;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> new file mode 100644
> index 0000000..f29f8ce
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright © 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.
> + *
> + */
> +#ifndef _INTEL_GUC_H_
> +#define _INTEL_GUC_H_
> +
> +#include "intel_guc_fwif.h"
> +#include "i915_guc_reg.h"
> +#include "intel_ringbuffer.h"
> +#include "intel_guc_ct.h"
> +#include "i915_vma.h"
> +
> +struct drm_i915_gem_request;
> +
> +/*
> + * This structure primarily describes the GEM object shared with the GuC.
> + * The specs sometimes refer to this object as a "GuC context", but we use
> + * the term "client" to avoid confusion with hardware contexts. This
> + * GEM object is held for the entire lifetime of our interaction with
> + * the GuC, being allocated before the GuC is loaded with its firmware.
> + * Because there's no way to update the address used by the GuC after
> + * initialisation, the shared object must stay pinned into the GGTT as
> + * long as the GuC is in use. We also keep the first page (only) mapped
> + * into kernel address space, as it includes shared data that must be
> + * updated on every request submission.
> + *
> + * The single GEM object described here is actually made up of several
> + * separate areas, as far as the GuC is concerned. The first page (kept
> + * kmap'd) includes the "process descriptor" which holds sequence data for
> + * the doorbell, and one cacheline which actually *is* the doorbell; a
> + * write to this will "ring the doorbell" (i.e. send an interrupt to the
> + * GuC). The subsequent  pages of the client object constitute the work
> + * queue (a circular array of work items), again described in the process
> + * descriptor. Work queue pages are mapped momentarily as required.
> + *
> + * We also keep a few statistics on failures. Ideally, these should all
> + * be zero!
> + *   no_wq_space: times that the submission pre-check found no space was
> + *                available in the work queue (note, the queue is shared,
> + *                not per-engine). It is OK for this to be nonzero, but
> + *                it should not be huge!
> + *   b_fail: failed to ring the doorbell. This should never happen, unless
> + *           somehow the hardware misbehaves, or maybe if the GuC firmware
> + *           crashes? We probably need to reset the GPU to recover.
> + *   retcode: errno from last guc_submit()
> + */
> +struct i915_guc_client {
> +	struct i915_vma *vma;
> +	void *vaddr;
> +	struct i915_gem_context *owner;
> +	struct intel_guc *guc;
> +
> +	uint32_t engines;		/* bitmap of (host) engine ids	*/
> +	uint32_t priority;
> +	u32 stage_id;
> +	uint32_t proc_desc_offset;
> +
> +	u16 doorbell_id;
> +	unsigned long doorbell_offset;
> +	u32 doorbell_cookie;
> +
> +	spinlock_t wq_lock;
> +	uint32_t wq_offset;
> +	uint32_t wq_size;
> +	uint32_t wq_tail;
> +	uint32_t wq_rsvd;
> +	uint32_t no_wq_space;
> +
> +	/* Per-engine counts of GuC submissions */
> +	uint64_t submissions[I915_NUM_ENGINES];
> +};
> +
> +struct intel_guc_log {
> +	uint32_t flags;
> +	struct i915_vma *vma;
> +	/* The runtime stuff gets created only when GuC logging gets enabled */
> +	struct {
> +		void *buf_addr;
> +		struct workqueue_struct *flush_wq;
> +		struct work_struct flush_work;
> +		struct rchan *relay_chan;
> +	} runtime;
> +	/* logging related stats */
> +	u32 capture_miss_count;
> +	u32 flush_interrupt_count;
> +	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
> +	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
> +	u32 flush_count[GUC_MAX_LOG_BUFFER];
> +};
> +
> +struct intel_guc {
> +	struct intel_uc_fw fw;
> +	struct intel_guc_log log;
> +	struct intel_guc_ct ct;
> +
> +	/* 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);
> +	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
> +
> +	/* 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);
> +};
> +
> +/* intel_guc.c */
> +inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i);

Why?

> +void guc_capture_load_err_log(struct intel_guc *guc);
> +void guc_free_load_err_log(struct intel_guc *guc);
> +int guc_enable_communication(struct intel_guc *guc);
> +void guc_disable_communication(struct intel_guc *guc);

Likely also not needed.
Btw, public functions shall start with "intel_guc_" prefix.

> +void intel_guc_init_early(struct intel_guc *guc);
> +int intel_guc_sample_forcewake(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);
> +
> +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);
> +}
> +
> +/* intel_guc_loader.c */
> +int intel_guc_select_fw(struct intel_guc *guc);
> +int intel_guc_init_hw(struct intel_guc *guc);
> +int intel_guc_suspend(struct drm_i915_private *dev_priv);
> +int intel_guc_resume(struct drm_i915_private *dev_priv);
> +u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> +
> +/* i915_guc_submission.c */
> +int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> +int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> +int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
> +void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
> +void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> +
> +/* intel_guc_log.c */
> +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);
> +
> +static inline u32 guc_ggtt_offset(struct i915_vma *vma)

Placement of this function here, suggests that it is related to guc_log.h
Maybe moving it right after struct intel_guc definition will be better ?

> +{
> +	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;
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8b0ae7f..81e03a6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -27,7 +27,6 @@
>   *    Alex Dai <yu.dai at intel.com>
>   */
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  /**
>   * DOC: GuC-specific firmware loader
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 6145fa0..8db3d5a 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -21,9 +21,7 @@
>   * IN THE SOFTWARE.
>   *
>   */
> -#include <linux/firmware.h>
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  
>  /**
>   * DOC: HuC Firmware
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> new file mode 100644
> index 0000000..4c46857
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright © 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.
> + *
> + */
> +#ifndef _INTEL_HUC_H_
> +#define _INTEL_HUC_H_
> +
> +struct intel_huc {
> +	/* Generic uC firmware management */
> +	struct intel_uc_fw fw;
> +
> +	/* HuC-specific additions */
> +};
> +
> +/* 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);

Hmm, I'm still having a problem with function...
Maybe we should convert it into

	void intel_guc_auth_huc(struct intel_guc *guc,
				struct intel_uc_fw *fw)
	{
	}

and move to intel_guc.c, or rename into

	void intel_huc_auth(struct intel_huc *huc)
	{
	}

and use intel_guc_auth_huc() only to wrap send() code ?


> +#endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 27e072c..95af45f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -23,7 +23,6 @@
>   */
>  
>  #include "i915_drv.h"
> -#include "intel_uc.h"
>  #include <linux/firmware.h>
>  
>  /* Cleans up uC firmware by releasing the firmware GEM obj.
> @@ -94,22 +93,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>  }
>  
> -static void guc_write_irq_trigger(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> -}
> -
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
>  
> -	intel_guc_ct_init_early(&guc->ct);
> -
> -	mutex_init(&guc->send_mutex);
> -	guc->send = intel_guc_send_nop;
> -	guc->notify = guc_write_irq_trigger;
> +	intel_guc_init_early(guc);
>  }
>  
>  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> @@ -262,72 +250,6 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
>  
> -static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> -{
> -	GEM_BUG_ON(!guc->send_regs.base);
> -	GEM_BUG_ON(!guc->send_regs.count);
> -	GEM_BUG_ON(i >= guc->send_regs.count);
> -
> -	return _MMIO(guc->send_regs.base + 4 * i);
> -}
> -
> -static void guc_init_send_regs(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	enum forcewake_domains fw_domains = 0;
> -	unsigned int i;
> -
> -	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> -	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> -
> -	for (i = 0; i < guc->send_regs.count; i++) {
> -		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> -					guc_send_reg(guc, i),
> -					FW_REG_READ | FW_REG_WRITE);
> -	}
> -	guc->send_regs.fw_domains = fw_domains;
> -}
> -
> -static void guc_capture_load_err_log(struct intel_guc *guc)
> -{
> -	if (!guc->log.vma || i915.guc_log_level < 0)
> -		return;
> -
> -	if (!guc->load_err_log)
> -		guc->load_err_log = i915_gem_object_get(guc->log.vma->obj);
> -
> -	return;
> -}
> -
> -static void guc_free_load_err_log(struct intel_guc *guc)
> -{
> -	if (guc->load_err_log)
> -		i915_gem_object_put(guc->load_err_log);
> -}
> -
> -static int guc_enable_communication(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	guc_init_send_regs(guc);
> -
> -	if (HAS_GUC_CT(dev_priv))
> -		return intel_guc_enable_ct(guc);
> -
> -	guc->send = intel_guc_send_mmio;
> -	return 0;
> -}
> -
> -static void guc_disable_communication(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	if (HAS_GUC_CT(dev_priv))
> -		intel_guc_disable_ct(guc);
> -
> -	guc->send = intel_guc_send_nop;
> -}
> -
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -458,82 +380,3 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  
>  	i915_ggtt_disable_guc(dev_priv);
>  }
> -
> -int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> -{
> -	WARN(1, "Unexpected send: action=%#x\n", *action);
> -	return -ENODEV;
> -}
> -
> -/*
> - * This function implements the MMIO based host to GuC interface.
> - */
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 status;
> -	int i;
> -	int ret;
> -
> -	GEM_BUG_ON(!len);
> -	GEM_BUG_ON(len > guc->send_regs.count);
> -
> -	/* If CT is available, we expect to use MMIO only during init/fini */
> -	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
> -		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> -		*action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> -
> -	mutex_lock(&guc->send_mutex);
> -	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
> -
> -	for (i = 0; i < len; i++)
> -		I915_WRITE(guc_send_reg(guc, i), action[i]);
> -
> -	POSTING_READ(guc_send_reg(guc, i - 1));
> -
> -	intel_guc_notify(guc);
> -
> -	/*
> -	 * No GuC command should ever take longer than 10ms.
> -	 * Fast commands should still complete in 10us.
> -	 */
> -	ret = __intel_wait_for_register_fw(dev_priv,
> -					   guc_send_reg(guc, 0),
> -					   INTEL_GUC_RECV_MASK,
> -					   INTEL_GUC_RECV_MASK,
> -					   10, 10, &status);
> -	if (status != INTEL_GUC_STATUS_SUCCESS) {
> -		/*
> -		 * Either the GuC explicitly returned an error (which
> -		 * we convert to -EIO here) or no response at all was
> -		 * received within the timeout limit (-ETIMEDOUT)
> -		 */
> -		if (ret != -ETIMEDOUT)
> -			ret = -EIO;
> -
> -		DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
> -			 " ret=%d status=0x%08X response=0x%08X\n",
> -			 action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
> -	}
> -
> -	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> -	mutex_unlock(&guc->send_mutex);
> -
> -	return ret;
> -}
> -
> -int intel_guc_sample_forcewake(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 action[2];
> -
> -	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
> -	/* WaRsDisableCoarsePowerGating:skl,bxt */
> -	if (!intel_enable_rc6() || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
> -		action[1] = 0;
> -	else
> -		/* bit 0 and 1 are for Render and Media domain separately */
> -		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
> -
> -	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> -}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 22ae52b..13460f8 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -24,72 +24,6 @@
>  #ifndef _INTEL_UC_H_
>  #define _INTEL_UC_H_
>  
> -#include "intel_guc_fwif.h"
> -#include "i915_guc_reg.h"
> -#include "intel_ringbuffer.h"
> -#include "intel_guc_ct.h"
> -#include "i915_vma.h"
> -
> -struct drm_i915_gem_request;
> -
> -/*
> - * This structure primarily describes the GEM object shared with the GuC.
> - * The specs sometimes refer to this object as a "GuC context", but we use
> - * the term "client" to avoid confusion with hardware contexts. This
> - * GEM object is held for the entire lifetime of our interaction with
> - * the GuC, being allocated before the GuC is loaded with its firmware.
> - * Because there's no way to update the address used by the GuC after
> - * initialisation, the shared object must stay pinned into the GGTT as
> - * long as the GuC is in use. We also keep the first page (only) mapped
> - * into kernel address space, as it includes shared data that must be
> - * updated on every request submission.
> - *
> - * The single GEM object described here is actually made up of several
> - * separate areas, as far as the GuC is concerned. The first page (kept
> - * kmap'd) includes the "process descriptor" which holds sequence data for
> - * the doorbell, and one cacheline which actually *is* the doorbell; a
> - * write to this will "ring the doorbell" (i.e. send an interrupt to the
> - * GuC). The subsequent  pages of the client object constitute the work
> - * queue (a circular array of work items), again described in the process
> - * descriptor. Work queue pages are mapped momentarily as required.
> - *
> - * We also keep a few statistics on failures. Ideally, these should all
> - * be zero!
> - *   no_wq_space: times that the submission pre-check found no space was
> - *                available in the work queue (note, the queue is shared,
> - *                not per-engine). It is OK for this to be nonzero, but
> - *                it should not be huge!
> - *   b_fail: failed to ring the doorbell. This should never happen, unless
> - *           somehow the hardware misbehaves, or maybe if the GuC firmware
> - *           crashes? We probably need to reset the GPU to recover.
> - *   retcode: errno from last guc_submit()
> - */
> -struct i915_guc_client {
> -	struct i915_vma *vma;
> -	void *vaddr;
> -	struct i915_gem_context *owner;
> -	struct intel_guc *guc;
> -
> -	uint32_t engines;		/* bitmap of (host) engine ids	*/
> -	uint32_t priority;
> -	u32 stage_id;
> -	uint32_t proc_desc_offset;
> -
> -	u16 doorbell_id;
> -	unsigned long doorbell_offset;
> -	u32 doorbell_cookie;
> -
> -	spinlock_t wq_lock;
> -	uint32_t wq_offset;
> -	uint32_t wq_size;
> -	uint32_t wq_tail;
> -	uint32_t wq_rsvd;
> -	uint32_t no_wq_space;
> -
> -	/* Per-engine counts of GuC submissions */
> -	uint64_t submissions[I915_NUM_ENGINES];
> -};
> -
>  enum intel_uc_fw_status {
>  	INTEL_UC_FIRMWARE_FAIL = -1,
>  	INTEL_UC_FIRMWARE_NONE = 0,
> @@ -156,69 +90,6 @@ struct intel_uc_fw {
>  	uint32_t ucode_offset;
>  };
>  
> -struct intel_guc_log {
> -	uint32_t flags;
> -	struct i915_vma *vma;
> -	/* The runtime stuff gets created only when GuC logging gets enabled */
> -	struct {
> -		void *buf_addr;
> -		struct workqueue_struct *flush_wq;
> -		struct work_struct flush_work;
> -		struct rchan *relay_chan;
> -	} runtime;
> -	/* logging related stats */
> -	u32 capture_miss_count;
> -	u32 flush_interrupt_count;
> -	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 flush_count[GUC_MAX_LOG_BUFFER];
> -};
> -
> -struct intel_guc {
> -	struct intel_uc_fw fw;
> -	struct intel_guc_log log;
> -	struct intel_guc_ct ct;
> -
> -	/* 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);
> -	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
> -
> -	/* 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);
> -};
> -
> -struct intel_huc {
> -	/* Generic uC firmware management */
> -	struct intel_uc_fw fw;
> -
> -	/* HuC-specific additions */
> -};
> -
>  /* intel_uc.c */
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
> @@ -226,54 +97,5 @@ struct intel_huc {
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> -int intel_guc_sample_forcewake(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);
> -
> -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);
> -}
> -
> -/* intel_guc_loader.c */
> -int intel_guc_select_fw(struct intel_guc *guc);
> -int intel_guc_init_hw(struct intel_guc *guc);
> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -int intel_guc_resume(struct drm_i915_private *dev_priv);
> -u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> -
> -/* i915_guc_submission.c */
> -int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> -int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> -int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
> -void i915_guc_wq_unreserve(struct drm_i915_gem_request *request);
> -void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> -struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
> -
> -/* intel_guc_log.c */
> -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);
> -
> -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;
> -}
> -
> -/* 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);
>  
>  #endif
> -- 
> 1.9.1
> 


More information about the Intel-gfx mailing list