On 02/06/2018 01:11 PM, Michal Wajdeczko wrote:
On Tue, 06 Feb 2018 06:15:54 +0100, Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> wrote:On 2/6/2018 5:32 AM, Jackie Li wrote:intel_guc_reg.h should only include definition for GuC registers and related register bits. Non-register related GuC WOPCM macro definitions should not be defined in intel_guc_reg.hThis patch creates a better file structure by moving non-register related GuC WOPCM macro definitions into a new header intel_guc_wopcm.h and movingGuC WOPCM related functions to a new source file intel_guc_wopcm.c asfuture patches will increase the complexity of determining the GuC WOPCMoffset and size.Hmm, I'm not sure that we need such low-details explanation that repeats filenames listed below. Maybe it can like this: "New file structure is needed as future patches will increase the complexity of determining the GuC WOPCM offset and size."v8:- Fixed naming, coding style issues and typo in commit message (Sagar) - Updated commit message to explain why we need create new file for GuCWOPCM related code (Chris) Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> (v7) Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>Minor change in function comment needed below as per kernel-doc format. Otherwise, change is good. Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>--- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/intel_guc.c | 11 -------- drivers/gpu/drm/i915/intel_guc.h | 2 +- drivers/gpu/drm/i915/intel_guc_reg.h | 4 ---drivers/gpu/drm/i915/intel_guc_wopcm.c | 46 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_guc_wopcm.h | 39 ++++++++++++++++++++++++++++drivers/gpu/drm/i915/intel_uc.c | 2 +- drivers/gpu/drm/i915/intel_uc_fw.c | 2 +- 8 files changed, 89 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_guc_wopcm.c create mode 100644 drivers/gpu/drm/i915/intel_guc_wopcm.hdiff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefileindex 3bddd8a..1dc9988 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -88,6 +88,7 @@ i915-y += intel_uc.o \ intel_guc_fw.o \ intel_guc_log.o \ intel_guc_submission.o \ + intel_guc_wopcm.o \ intel_huc.o # autogenerated null render statediff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.cindex 21140cc..9f45e6d 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c@@ -509,14 +509,3 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)i915_gem_object_put(obj); return vma; } - -u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv) -{ - u32 wopcm_size = GUC_WOPCM_TOP; - - /* On BXT, the top of WOPCM is reserved for RC6 context */ - if (IS_GEN9_LP(dev_priv)) - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED; - - return wopcm_size; -}diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.hindex 52856a9..9e0a97e 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -31,6 +31,7 @@ #include "intel_guc_ct.h" #include "intel_guc_log.h" #include "intel_guc_reg.h" +#include "intel_guc_wopcm.h" #include "intel_uc_fw.h" #include "i915_vma.h"@@ -130,6 +131,5 @@ 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);-u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); #endifdiff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.hindex 19a9247..1f52fb8 100644 --- a/drivers/gpu/drm/i915/intel_guc_reg.h +++ b/drivers/gpu/drm/i915/intel_guc_reg.h @@ -68,7 +68,6 @@ #define DMA_GUC_WOPCM_OFFSET _MMIO(0xc340) #define HUC_LOADING_AGENT_VCR (0<<1) #define HUC_LOADING_AGENT_GUC (1<<1) -#define GUC_WOPCM_OFFSET_VALUE 0x80000 /* 512KB */ #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4) #define HUC_STATUS2 _MMIO(0xD3B0) @@ -76,9 +75,6 @@ /* Defines WOPCM space available to GuC firmware */ #define GUC_WOPCM_SIZE _MMIO(0xc050) -/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */ -#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */ -#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB *//* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */#define GUC_GGTT_TOP 0xFEE00000diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.cnew file mode 100644 index 0000000..73be9af --- /dev/null +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c @@ -0,0 +1,46 @@ +/* + * Copyright © 2017 Intel Corporation2017-2018+ *+ * 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.Hmm, recently there was a discussion about using SPDX identifiers, so maybe:+/* + * SPDX-License-Identifier: MIT + * + * Copyright © 2017-2018 Intel Corporation + */
Thanks for the heads up, will update it.
+ * + */ + +#include "intel_guc_wopcm.h" +#include "i915_drv.h" + +/*Should be "/**"+ * intel_guc_wopcm_size() - Get the size of GuC WOPCM. + * @guc: intel guc. + * + * Get the platform specific GuC WOPCM size. + * + * Return: size of the GuC WOPCM. + */ +u32 intel_guc_wopcm_size(struct intel_guc *guc) +{ + struct drm_i915_private *i915 = guc_to_i915(guc); + u32 size = GUC_WOPCM_TOP; + + /* On BXT, the top of WOPCM is reserved for RC6 context */ + if (IS_GEN9_LP(i915)) + size -= BXT_GUC_WOPCM_RC6_RESERVED; + + return size; +}diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.hnew file mode 100644 index 0000000..53de931 --- /dev/null +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h @@ -0,0 +1,39 @@ +/* + * 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. + * + */Use SPDX here too+ +#ifndef _INTEL_GUC_WOPCM_H_ +#define _INTEL_GUC_WOPCM_H_ + +#include <linux/types.h> + +struct intel_guc; + +#define GUC_WOPCM_OFFSET_VALUE 0x80000 /* 512KB */a) maybe we can write it as (512 * 1024) to avoid need for comment ?
I prefer to keep it as 0x80000 to avoid some extra calculation.
b) is this value related somehow to GUC_WOPCM_TOP defined below ? c) if not add description
it's a static value we currently use in code.
Actually, I've already had some new thoughts about this. Will submit new patches later:-)+/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */ +#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */ +#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */ + +u32 intel_guc_wopcm_size(struct intel_guc *guc); + +#endifdiff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.cindex 9f1bac6..1b2831b 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c@@ -340,7 +340,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)gen9_reset_guc_interrupts(dev_priv); /* init WOPCM */ - I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv)); + I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(guc)); I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.cindex 784eff9..24945cf 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/intel_uc_fw.c@@ -97,7 +97,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,/* Header and uCode will be loaded to WOPCM */ size = uc_fw->header_size + uc_fw->ucode_size; - if (size > intel_guc_wopcm_size(dev_priv)) { + if (size > intel_guc_wopcm_size(&dev_priv->guc)) {Hmm, I hope this will be moved to GuC specific loader or completely removedin future patch as "uc" functions shall not use "guc" specific functions.
DRM_WARN("%s: Firmware is too large to fit in WOPCM\n", intel_uc_fw_type_repr(uc_fw->type)); err = -E2BIG;
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
