Re: [Intel-gfx] [PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/19/2018 08:45 AM, Michal Wajdeczko wrote:
On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li  
wrote:



After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set to 3
(enable GuC and HuC firmware loading) if the module was originally 
loaded

with enable_guc set to 1 (only enable GuC firmware loading). This is
because WOPCM registers were updated and locked without considering 
the HuC

FW size. Since we need both GuC and HuC FW sizes to determine the final
layout of WOPCM, we should always calculate the WOPCM layout based on 
the
actual sizes of the GuC and HuC firmware available for a specific 
platform

if we need continue to support enable/disable HuC FW loading dynamically
with enable_guc modparam.

This patch splits uC firmware fetching into two stages. First stage 
is to
fetch the firmware image and verify the firmware header. uC firmware 
will

be marked as verified and this will make FW info available for following
WOPCM layout calculation. The second stage is to create a GEM object and
copy the FW data into the created GEM object which will only be 
available
when GuC/HuC loading is enabled by enable_guc modparam. This will 
guarantee
that the WOPCM layout will be always be calculated correctly without 
making

any assumptions to the GuC and HuC firmware sizes.

v3:
 - Rebase

v4:
 - Renamed the new parameter add to intel_uc_fw_fetch (Michal)

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
Reviewed-by: John Spotswood 
---
 drivers/gpu/drm/i915/intel_uc.c    | 14 --
 drivers/gpu/drm/i915/intel_uc_fw.c | 31 ---
 drivers/gpu/drm/i915/intel_uc_fw.h |  7 +--
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c

index 1cffaf7..73b8f6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private 
*i915)

sanitize_options_early(i915);
-    if (USES_GUC(i915))
-    intel_uc_fw_fetch(i915, >fw);
-
-    if (USES_HUC(i915))
-    intel_uc_fw_fetch(i915, >fw);
+    intel_uc_fw_fetch(i915, >fw, USES_GUC(i915));
+    intel_uc_fw_fetch(i915, >fw, USES_HUC(i915));


Again: I'm don't think that unconditional fetch of HuC fw is a right 
choice.
We should look for other options how to support enable_guc 1-->3 
scenario.


This is the real fix I can think of to support such a scenario. I think 
the performance
impact here is minimal since it's only one time operation. I will think 
about the

use case of HAS_HUC = 1 and only enable guc submission.

But first I suggest we need to define the expected use case (like I 
mentioned in my last reply):
how to define "support enable_guc 1->3" (whether we should expect some 
system reboot, or
we need guarantee 100% work with no system reboot required)? whether a 
system reboot for

FW changes should be treated as code defects, etc.

 }
void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct 
drm_i915_private *i915)

 struct intel_guc *guc = >guc;
 struct intel_huc *huc = >huc;
-    if (USES_HUC(i915))
-    intel_uc_fw_fini(>fw);
-
-    if (USES_GUC(i915))
-    intel_uc_fw_fini(>fw);
+    intel_uc_fw_fini(>fw);
+    intel_uc_fw_fini(>fw);
guc_free_load_err_log(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
b/drivers/gpu/drm/i915/intel_uc_fw.c

index 6e8e0b5..c1fed06 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -33,11 +33,13 @@
  *
  * @dev_priv: device private
  * @uc_fw: uC firmware
+ * @fetch: whether fetch uC firmware into GEM object or not
  *
- * Fetch uC firmware into GEM obj.
+ * Fetch and verify uC firmware and copy firmware data into GEM 
object if

+ * @fetch is true.
  */
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-   struct intel_uc_fw *uc_fw)
+   struct intel_uc_fw *uc_fw, bool fetch)
 {
 struct pci_dev *pdev = dev_priv->drm.pdev;
 struct drm_i915_gem_object *obj;
@@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private 
*dev_priv,

 goto fail;
 }
-    obj = i915_gem_object_create_from_data(dev_priv, fw->data, 
fw->size);

-    if (IS_ERR(obj)) {
-    err = PTR_ERR(obj);
-    DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
- intel_uc_fw_type_repr(uc_fw->type), err);
-    goto fail;
+    uc_fw->size = fw->size;
+    uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;


btw, I'm not sure that this new state is needed at all, as you 

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li]

On 04/19/2018 09:37 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong...@intel.com> 
wrote:



On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li <yaodong...@intel.com> 
wrote:


In current code, we only compare the locked WOPCM register values 
with the
calculated values. However, we can continue loading GuC/HuC 
firmware if the
locked (or partially locked) values were valid for current GuC/HuC 
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will 
recalculate the
verify the new values if these registers were partially locked, so 
that we
won't fail the GuC/HuC firmware loading even if the locked register 
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li <yaodong...@intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kam...@intel.com>
Cc: Michal Winiarski <michal.winiar...@intel.com>
Cc: John Spotswood <john.a.spotsw...@intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217 
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,

 return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+    return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+ GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+    return guc_fw_size + GUC_WOPCM_RESERVED + 
GUC_WOPCM_STACK_RESERVED;

+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm 
*wopcm,

+   u32 guc_wopcm_base,
+   u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+    u32 ctx_rsvd = context_reserved_size(i915);
+
+    if (guc_wopcm_base >= wopcm->size ||
+    (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+    return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies 
values.



+    }
+
+    *guc_wopcm_size =
+    (wopcm->size - guc_wopcm_base - ctx_rsvd) & 
GUC_WOPCM_SIZE_MASK;

+
+    return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work 
with

both calculated and locked values?
I actually thought about that. However, since the verification based 
on the assumption
that the unlocked reg value could be any numbers so it puts more 
checks on these values

which are unnecessary during the calculation.


maybe some checks would be "unnecessary" but they still should be valid.
and code reuse is nice benefit that sacrifice that few extra checks.
Hmm, actually there's no duplicated code here. But I agree that it will 
be clearer to
have single place for the verification - it makes sense too:). So will 
choose to

sacrifice a little bit time here for unnecessary checks.



I've made the responding check common
enough to be reused by this verification code and the calculation code.



*i915,
+   u32 guc_fw_size, u32 huc_fw_size,
+   u32 guc_wopcm_base,
+   u32 guc_wopcm_size)
+{
+    if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+    DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+  calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice

Will update it.



+  guc_wopcm_size / 1024);
+    return -E2BIG;
+    }
+
+    return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+    huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
 u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-    u32 ctx_rsvd = context_reserved_size(i915);
 u32 guc_wopcm_base;
 u32 guc_wopcm_size;
-    u32 guc_wopcm_rsvd;
 int err;
    GEM_BUG_ON(!w

Re: [Intel-gfx] [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Yaodong Li]

On 04/19/2018 08:52 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 19:43:52 +0200, Yaodong Li <yaodong...@intel.com> 
wrote:



On 04/13/2018 07:26 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li <yaodong...@intel.com> 
wrote:



The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
dynamcially during i915 module loading. If WOPCM offset register was

  
typo


D'oh! I really need a tool for this! Thanks, will fix it.

locked
without having HUC_LOADING_AGENT_GUC bit set to 1, the module 
reloading
with both GuC and HuC FW will fail since we need to set this bit to 
1 for

HuC FW uploading.

Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, 
this

patch updates the register updating code to make sure the WOPCM offset
register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 
which
will guarantee successful uploading of both GuC and HuC FW. We will 
further

take care of the locked values in the following enhancement patch.

Signed-off-by: Jackie Li <yaodong...@intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kam...@intel.com>
Cc: Michal Winiarski <michal.winiar...@intel.com>
Cc: John Spotswood <john.a.spotsw...@intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index 74bf76f..b1c08ca 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -238,8 +238,6 @@ static inline int write_and_verify(struct 
drm_i915_private *dev_priv,

 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-    u32 huc_agent;
-    u32 mask;
 int err;
    if (!USES_GUC(dev_priv))
@@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm 
*wopcm)

 if (err)
 goto err_out;
-    huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
-    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | 
huc_agent;

 err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
-   wopcm->guc.base | huc_agent, mask,
+   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
+   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
+   GUC_WOPCM_OFFSET_VALID,


while we can unconditionally set HUC_AGENT bit, there is no need to 
verify

it unless we are using HuC, so we can consider leaving old mask intact.
The idea is to verify the written values are exactly we want, so I 
think it better

to keep doing it in this way.


Hmm, but then instead of being more flexible, you're unnecessary 
restricting

yourself to require HUC_AGENT bit, even if you don't need it - recall the
theoretical scenario with bad bios that already locked that register.


Hmm. Actually my thought is pretty simple here. we want to always set this
bit so we always keep checking it. For the fault bios handling, my 
thought is

if this reg was locked without HUC_AGENT bit when USES_HUC is true. we will
return error - the 3/3 patch is taking care of this.


This seems to be little inconsistent with earlier patch where you try to
support much more different scenario (from no HuC fw to use HuC fw)

My 1/3 patch is trying to support enable_guc=1->3->1 without any FW changes
on the FS while work as an enhancement patch to handle more complicated
cases for the theoretical scenario - faulty bios.

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/19/2018 08:31 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong...@intel.com> 
wrote:



On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong...@intel.com> 
wrote:



After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set 
to 3
(enable GuC and HuC firmware loading) if the module was originally 
loaded

with enable_guc set to 1 (only enable GuC firmware loading).


Is this frequent and required scenario ? or just for 
debug/development ?


My understanding is this should be a nice to have feature and mainly 
for debugging.

This is
because WOPCM registers were updated and locked without considering 
the HuC
FW size. Since we need both GuC and HuC FW sizes to determine the 
final
layout of WOPCM, we should always calculate the WOPCM layout based 
on the
actual sizes of the GuC and HuC firmware available for a specific 
platform
if we need continue to support enable/disable HuC FW loading 
dynamically

with enable_guc modparam.

This patch splits uC firmware fetching into two stages. First stage 
is to
fetch the firmware image and verify the firmware header. uC 
firmware will
be marked as verified and this will make FW info available for 
following
WOPCM layout calculation. The second stage is to create a GEM 
object and
copy the FW data into the created GEM object which will only be 
available
when GuC/HuC loading is enabled by enable_guc modparam. This will 
guarantee
that the WOPCM layout will be always be calculated correctly 
without making

any assumptions to the GuC and HuC firmware sizes.


You are also assuming that on reload exactly the same GuC/HuC firmwares
will bee used as in initial run. This will make this useless for debug/
development scenarios, where custom fw are likely to be specified.

This patch is mainly for providing a real fix to support 
enable_guc=1->3->1 use case.
It based on the fact that it is inevitable that sometimes we need to 
reboot the system

if the status of the fw was changed on the file system.


What do you mean by "status of the fw was changed on the file system" ?
* change of the fw binary/version/size, or
* change from not-present to present ?

I think it should include all of the presence changes, file updates.


I am not sure how often we switch between different HuC FW with 
different sizes?


Just above you said that you need this "mainly for debugging" so
I would expect that then different fw sizes are expected.


If we want to support enable_guc=1->3->1 scenarios for debug/dev then
maybe more flexible will be other approach that makes allocations from
the other end as proposed in [1]

[1] https://patchwork.freedesktop.org/patch/212471/
Actually, I do think this might be one of the options, and I've also 
put some comments on this
series. The main concern I have is it still make assumption on the 
GuC FW size and may


But in enable_guc=1-->3 scenario, I would assume that the only difference
will be HuC fw (as with enable=1 we already loaded GuC)
Hmm, my main concern to the current "from the end" solution is it makes 
assumption on

the GuC FW size in order to meet the HW restriction.


If you want just to test different GuC fws, then it is different scenario
as then enable_guc will always be = 1.

what I mean is the "from the end" approach will lead to the same issue 
for different GuC FW sizes - we
may have to reboot the system for GuC FW debugging (different GuC FW 
sizes) even if enable_guc is always
set to 1. However, with the current "from the beginning" way we won't 
run into such problems
for GuC FW debugging (since it's already used the max available space). 
Thus I think we should

define the enable_guc = 1->3->1 as following:
we would support use of enable_guc=1->3->1 correctly without system 
reboot for the present FWs. A system
reboot will be expected (but not necessarily happen if we found current 
partition works for the new FWs)

for any FW changes (including add/remove/update).

if we decide to drop the support for enable_guc=1->3->1 since it's only 
for debugging purpose then we should
expect a system reboot for either "from the end" or "from the beginning" 
solutions since we cannot 100% have
this issue (changing FW without a system boot) solved. Therefore, the 
require of system reboot should not be

a bug when it comes to FW updating.


run into the same issue if the GuC FW failed to meet the requirement.
and for debugging purpose it would have the same possible for 
different GuC FW debugging.




v3:
 - Rebase

Signed-off-by: Jackie Li <yaodong...@intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kam...@intel.com>
Cc: Michal Winiarski <michal.winiar...@intel.com>
Cc: Joh

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li]

On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li  
wrote:


In current code, we only compare the locked WOPCM register values 
with the
calculated values. However, we can continue loading GuC/HuC firmware 
if the
locked (or partially locked) values were valid for current GuC/HuC 
firmware

sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so 
that we
won't fail the GuC/HuC firmware loading even if the locked register 
values

are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 217 
+++--

 1 file changed, 185 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index b1c08ca..fa8d2be 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,

 return err;
 }
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+    return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+ GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+    return guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm 
*wopcm,

+   u32 guc_wopcm_base,
+   u32 *guc_wopcm_size)


Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.


+{
+    struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+    u32 ctx_rsvd = context_reserved_size(i915);
+
+    if (guc_wopcm_base >= wopcm->size ||
+    (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+    return -E2BIG;


You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies values.


+    }
+
+    *guc_wopcm_size =
+    (wopcm->size - guc_wopcm_base - ctx_rsvd) & 
GUC_WOPCM_SIZE_MASK;

+
+    return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private


hmm, maybe we can unify somehow this verification to be able to work with
both calculated and locked values?
I actually thought about that. However, since the verification based on 
the assumption
that the unlocked reg value could be any numbers so it puts more checks 
on these values
which are unnecessary during the calculation. I've made the responding 
check common

enough to be reused by this verification code and the calculation code.



*i915,
+   u32 guc_fw_size, u32 huc_fw_size,
+   u32 guc_wopcm_base,
+   u32 guc_wopcm_size)
+{
+    if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+    DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+  calculate_min_guc_wopcm_size(guc_fw_size),


you are calling calculate_min_guc_wopcm_size() twice

Will update it.



+  guc_wopcm_size / 1024);
+    return -E2BIG;
+    }
+
+    return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+    huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
 u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-    u32 ctx_rsvd = context_reserved_size(i915);
 u32 guc_wopcm_base;
 u32 guc_wopcm_size;
-    u32 guc_wopcm_rsvd;
 int err;
GEM_BUG_ON(!wopcm->size);
@@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 return -E2BIG;
 }
-    guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-   GUC_WOPCM_OFFSET_ALIGNMENT);
-    if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-    DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
-  guc_wopcm_base / 1024);
+    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+    err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+   _wopcm_size);
+    if (err)
+    return err;
+
+    DRM_DEBUG_DRIVER("Calculated GuC WOPCM 

Re: [Intel-gfx] [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Yaodong Li]

On 04/13/2018 07:26 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li  
wrote:



The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
dynamcially during i915 module loading. If WOPCM offset register was

  
typo


D'oh! I really need a tool for this! Thanks, will fix it.

locked
without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
with both GuC and HuC FW will fail since we need to set this bit to 1 
for

HuC FW uploading.

Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
patch updates the register updating code to make sure the WOPCM offset
register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
will guarantee successful uploading of both GuC and HuC FW. We will 
further

take care of the locked values in the following enhancement patch.

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index 74bf76f..b1c08ca 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -238,8 +238,6 @@ static inline int write_and_verify(struct 
drm_i915_private *dev_priv,

 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-    u32 huc_agent;
-    u32 mask;
 int err;
if (!USES_GUC(dev_priv))
@@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 if (err)
 goto err_out;
-    huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
-    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
 err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
-   wopcm->guc.base | huc_agent, mask,
+   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
+   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
+   GUC_WOPCM_OFFSET_VALID,


while we can unconditionally set HUC_AGENT bit, there is no need to 
verify

it unless we are using HuC, so we can consider leaving old mask intact.
The idea is to verify the written values are exactly we want, so I think 
it better

to keep doing it in this way.

Thanks,
Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li  
wrote:



After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set to 3
(enable GuC and HuC firmware loading) if the module was originally 
loaded

with enable_guc set to 1 (only enable GuC firmware loading).


Is this frequent and required scenario ? or just for debug/development ?

My understanding is this should be a nice to have feature and mainly for 
debugging.

This is
because WOPCM registers were updated and locked without considering 
the HuC

FW size. Since we need both GuC and HuC FW sizes to determine the final
layout of WOPCM, we should always calculate the WOPCM layout based on 
the
actual sizes of the GuC and HuC firmware available for a specific 
platform

if we need continue to support enable/disable HuC FW loading dynamically
with enable_guc modparam.

This patch splits uC firmware fetching into two stages. First stage 
is to
fetch the firmware image and verify the firmware header. uC firmware 
will

be marked as verified and this will make FW info available for following
WOPCM layout calculation. The second stage is to create a GEM object and
copy the FW data into the created GEM object which will only be 
available
when GuC/HuC loading is enabled by enable_guc modparam. This will 
guarantee
that the WOPCM layout will be always be calculated correctly without 
making

any assumptions to the GuC and HuC firmware sizes.


You are also assuming that on reload exactly the same GuC/HuC firmwares
will bee used as in initial run. This will make this useless for debug/
development scenarios, where custom fw are likely to be specified.

This patch is mainly for providing a real fix to support 
enable_guc=1->3->1 use case.
It based on the fact that it is inevitable that sometimes we need to 
reboot the system

if the status of the fw was changed on the file system.
I am not sure how often we switch between different HuC FW with 
different sizes?

If we want to support enable_guc=1->3->1 scenarios for debug/dev then
maybe more flexible will be other approach that makes allocations from
the other end as proposed in [1]

[1] https://patchwork.freedesktop.org/patch/212471/
Actually, I do think this might be one of the options, and I've also put 
some comments on this
series. The main concern I have is it still make assumption on the GuC 
FW size and may

run into the same issue if the GuC FW failed to meet the requirement.
and for debugging purpose it would have the same possible for different 
GuC FW debugging.




v3:
 - Rebase

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michal Winiarski 
Cc: John Spotswood 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_uc.c    | 14 --
 drivers/gpu/drm/i915/intel_uc_fw.c | 31 ---
 drivers/gpu/drm/i915/intel_uc_fw.h |  7 +--
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c

index 1cffaf7..73b8f6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private 
*i915)

sanitize_options_early(i915);
-    if (USES_GUC(i915))
-    intel_uc_fw_fetch(i915, >fw);
-
-    if (USES_HUC(i915))
-    intel_uc_fw_fetch(i915, >fw);
+    intel_uc_fw_fetch(i915, >fw, USES_GUC(i915));
+    intel_uc_fw_fetch(i915, >fw, USES_HUC(i915));


Hmm, side effect of those unconditional fetches might be unwanted 
warnings
about missing firmwares (on configs with disabled guc) as well as 
extended

driver load time.
Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will 
return directly.


Do we really need to support this corner case enable_guc=1->3 at all 
costs?
I think this is the real solution for this issue (with no assumption). 
However, we do
need to decide whether we should support such a corner case which is 
mainly for

debugging.


/michal


 }
void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct 
drm_i915_private *i915)

 struct intel_guc *guc = >guc;
 struct intel_huc *huc = >huc;
-    if (USES_HUC(i915))
-    intel_uc_fw_fini(>fw);
-
-    if (USES_GUC(i915))
-    intel_uc_fw_fini(>fw);
+    intel_uc_fw_fini(>fw);
+    intel_uc_fw_fini(>fw);
guc_free_load_err_log(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
b/drivers/gpu/drm/i915/intel_uc_fw.c

index 6e8e0b5..a9cb900 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -33,11 +33,13 @@
  *
  * @dev_priv: device private
  * @uc_fw: uC firmware
+ * @copy_to_obj: 

Re: [Intel-gfx] [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Yaodong Li]

On 03/26/2018 03:04 AM, Michał Winiarski wrote:

On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote:

On 03/23/2018 05:34 AM, Michał Winiarski wrote:

We need GuC to load HuC, but it's also possible for GuC to operate on
its own. We don't know what the size of HuC FW may be, so, not wanting
to make any platform-specific hardcoded guesses, we assume that its size
is 0... Which is a very bad approximation.
This has a very unfortunate consequence - once we've booted with GuC and
no HuC, we'll never be able to load HuC (unless we reboot, since the
registers are locked).
Rather than using unknown values in our computations - let's partition
based on GuC size.

we can do this based on known GuC and HuC sizes without
any assumption on FW sizes :)
please also refer to: https://patchwork.freedesktop.org/series/40541/

You need to have HuC FW present in the filesystem do do that though.
(IIUC we still get 0 if it's not there)

Yes. we cannot make any assumption to the status of the FW files as well.
so I think we should expect a system reboot for any FW updates.

We have one HW restriction where we're using HuC size (GuC size needs to
be roughly similar to HuC size - which may be unknown at this point),
luckily, another HW restriction makes it very unlikely to run in any
sort of issues in this case.

Signed-off-by: Michał Winiarski <michal.winiar...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Jackie Li <yaodong...@intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
---
   drivers/gpu/drm/i915/intel_wopcm.c | 60 
+-
   1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c
index 52841d340002..295d302e97b9 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, 
u32 ctx_rsvd)
return 0;
   }
-static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
+static inline void
+__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
+{
+   /*
+* We're growing guc region in the direction of lower addresses.
+* We need to use multiples of base alignment, because it has more
+* strict alignment rules.
+*/
+   size = DIV_ROUND_UP(size, 2);

A bit confused here. Don't we just want to push the base downward for
*size* bytes?

Starting from the following:

 +--+
 |--|
 ||||
 ||||
 ||  GuC   ||
 ||  region||
 ||||
 ||||
 ||||
 ||||
 +--+
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 +--+

We want to grow the whole region by size bytes. Pushing the base downward gives
us this:

 +--+
 |  Empty   |
 |  space   |
 +--+
 ||||
 ||||
 ||  GuC   ||
 ||  region||
 ||||
 ||||
 ||||
 ||||
 +--+
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 +--+

Which leaves less space for HuC (and we're also leaving a bunch of unused space
in our partitioning).

If we modify both base and size to end up with this:

 +--+
 |--|
 ||||
 ||||
 ||  GuC   ||
 ||  region||
 ||||
 ||||
 ||||
 ||||
 ||||
 +--+
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 |  |
 +--+

We're still satisfying the HW restriciton, but we're not wasting any space from
the top (and also we're leaving more space for HuC).


+   size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);

Hmm, I think we only need align it to 4K boundary.

No - because we're modifying both base (16K alignment) and size (4K
alignment).

Got it:-) Thanks!



+
+   wopcm->guc.base -= size;
+   wopcm->guc.size += size;
+}
+
+static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
   {
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
@@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm 
*wopcm)
size = gen9_size_fo

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li]

On 03/26/2018 12:15 AM, Joonas Lahtinen wrote:

Quoting Yaodong Li (2018-03-23 19:33:15)

As I said, I agree that we would likely solve the enable_guc=1 then
enable_guc=3 case with these changes which I think this the the only benefit
that we can get from the starting from the top way.
But my point is just like the from the bottom way, you are ignoring the
HuC FW size. e.g. you will need to grow the guc wopcm size for the hw
restrictions.
The problem is currently we do have this restriction on huc fw size v.s.
guc wopcm
size. In you solution, you are actually counting on the assumption that
guc fw size should be large enough so that we can ignore the huc fw size
and expect it still works when we set enable_huc=3. and my answer to
this is yes it will work for the cases that guc fw size is large enough, but
still risky to fail in case of guc fw size < huc_fw_size + 16K. and that
comes
to my point:
why not make life easier and accurate - If we decide to support dynamically
switching on/off huc fw loading and the fact we can get actual FW sizes,
why not drop all these assumptions and fix it in a way that we won't be
bothered by any FW side changes? :)

Is not the GuC vs. HuC FW size limitation going to be fixed for upcoming
platforms?

My gut instinct would be to partition based only on the enabled FW sizes,
whichever it be. Then just require a reboot if after that partitioning,
changing the parameter causes the FW not to fit.

That's my thought too. I think it makes sense to reboot the system for
any FW updates, especially when we have these write-once registers.

I believe the main reason we wanted to support enable_guc=1 (GuC FW only)
then enable_guc=3 (GuC + HuC FW) without a system reboot is to facilitate
the debugging.

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm/i915/guc: Don't touch WOPCM if we're not using GuC

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:

We probably shouldn't print out WOPCM size on platforms that don't have
GuC. We also want to make sure we don't hit any asserts if user explicitly
sets enable_guc != 0 on non-guc platforms.

Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Jackie Li 
Cc: Joonas Lahtinen 
Cc: Michal Wajdeczko 
---
  drivers/gpu/drm/i915/intel_wopcm.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c
index be8fca80aeca..828800ca119c 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -69,6 +69,9 @@
   */
  void intel_wopcm_init_early(struct intel_wopcm *wopcm)
  {
+   if (!HAS_GUC(wopcm_to_i915(wopcm)))
+   return;
+
wopcm->size = GEN9_WOPCM_SIZE;
  
  	DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024);

@@ -285,8 +288,12 @@ static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
   */
  int intel_wopcm_init(struct intel_wopcm *wopcm)
  {
+   struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
int err;
  
+	if (!HAS_GUC(dev_priv) || !USES_GUC(dev_priv))

+   return 0;
+

I guess I have to bring up an old question here: if we want to
use this only for enable_guc > 0. Why not make it as a part of
uc layer?

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:

We need GuC to load HuC, but it's also possible for GuC to operate on
its own. We don't know what the size of HuC FW may be, so, not wanting
to make any platform-specific hardcoded guesses, we assume that its size
is 0... Which is a very bad approximation.
This has a very unfortunate consequence - once we've booted with GuC and
no HuC, we'll never be able to load HuC (unless we reboot, since the
registers are locked).
Rather than using unknown values in our computations - let's partition
based on GuC size.

we can do this based on known GuC and HuC sizes without
any assumption on FW sizes :)
please also refer to: https://patchwork.freedesktop.org/series/40541/

We have one HW restriction where we're using HuC size (GuC size needs to
be roughly similar to HuC size - which may be unknown at this point),
luckily, another HW restriction makes it very unlikely to run in any
sort of issues in this case.

Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Jackie Li 
Cc: Joonas Lahtinen 
Cc: Michal Wajdeczko 
---
  drivers/gpu/drm/i915/intel_wopcm.c | 60 +-
  1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c
index 52841d340002..295d302e97b9 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, 
u32 ctx_rsvd)
return 0;
  }
  
-static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)

+static inline void
+__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
+{
+   /*
+* We're growing guc region in the direction of lower addresses.
+* We need to use multiples of base alignment, because it has more
+* strict alignment rules.
+*/
+   size = DIV_ROUND_UP(size, 2);

A bit confused here. Don't we just want to push the base downward for
*size* bytes?

+   size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);

Hmm, I think we only need align it to 4K boundary.

+
+   wopcm->guc.base -= size;
+   wopcm->guc.size += size;
+}
+
+static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
  {
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
@@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm 
*wopcm)
size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
   wopcm->guc.size);
if (size)
-   goto err;
+   __guc_region_grow(wopcm, size);
  
  		size = gen9_size_for_huc_restriction(wopcm->guc.size,

 huc_fw_size);

Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
Once the registers were locked, enable_guc=3 may still fail since 
huc_fw_size

may still large enough to break the restriction we want to enforce that:
hw_fw_size + 16KB > guc_fw_size.

if (size)
-   goto err;
-   }
-
-   return 0;
+   __guc_region_grow(wopcm, size);
  
-err:

-   DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
- wopcm->guc.size / 1024,
- size / 1024);
-
-   return -E2BIG;
+   GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
+  
wopcm->guc.size));
+   GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
+huc_fw_size));
+   }
  }
  
  static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)

@@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm 
*wopcm)
return 0;
  }
  
-static int wopcm_guc_init(struct intel_wopcm *wopcm)

+static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
  {
struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-   u32 huc_fw_size = intel_uc_fw_get_upload_size(_priv->huc.fw);
+   u32 guc_fw_size = intel_uc_fw_get_upload_size(_priv->guc.fw);
u32 ctx_rsvd = context_reserved_size(dev_priv);
  
-	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),

-GUC_WOPCM_OFFSET_ALIGNMENT);
+   wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
  
-	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,

-   PAGE_SIZE);
-
-   DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
-wopcm->guc.base / 1024,
-(wopcm->guc.base + wopcm->guc.size) / 1024);

Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Separate WOPCM partitioning from constraints check

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:

In the following patches we're going to support constraints checking on
an already locked partitioning. Let's structure the code now to allow
for code reuse and reduce the churn later on.

Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Jackie Li 
Cc: Joonas Lahtinen 
Cc: Michal Wajdeczko 
---
  drivers/gpu/drm/i915/intel_wopcm.c | 141 +++--
  1 file changed, 102 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c
index 50854a6b9493..52841d340002 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct 
drm_i915_private *i915)
return 0;
  }
  
+static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size)

+{
+   return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED +
+GUC_WOPCM_STACK_RESERVED, PAGE_SIZE);
+}
+
+static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size)
+{
+   return huc_fw_size + WOPCM_RESERVED_SIZE;
+}
+
  static u32
  gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
  {
@@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 
huc_fw_size)
return additional_size;
  }
  
-static inline int check_hw_restriction(struct drm_i915_private *i915,

-  u32 guc_wopcm_base, u32 guc_wopcm_size,
-  u32 huc_fw_size)
+static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size)

inline?

+{
+   if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) {
+   DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n",
+ huc_fw_size_in_wopcm(huc_fw_size) / 1024,
+ wopcm->guc.base / 1024);
+   return -E2BIG;
+   }
+
+   return 0;
+}
+
+static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size)

inline?

+{
+   if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) {
+   DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
+ huc_fw_size_in_wopcm(guc_fw_size) / 1024,
+ wopcm->guc.size / 1024);
+   return -E2BIG;
+   }
+
+   return 0;
+}
+
+static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)

inline?

+{
+   if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) {
+   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+ wopcm->guc.base / 1024);
+   return -E2BIG;
+   }
+
+   return 0;
+}
+
+static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
  {

We are repeating the the old discussion over the parameters here.

we wanted to keep wopcm contains either valid (non-zero) values
or to be zero all the time, so that we can make a check such as
if (!wopcm->guc.size) then it's valid. Originally, we needed check this
before accessing it in guc_ggtt_offset. Since we are not doing so
maybe we can change it back in this way, or just keep it as it is now
to continue gaining the benefit that it will continue contains valid data?

+   struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+   u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
u32 size;
  
  	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {

-   size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
-  guc_wopcm_size);
+   size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
+  wopcm->guc.size);
if (size)
goto err;
  
-		size = gen9_size_for_huc_restriction(guc_wopcm_size,

+   size = gen9_size_for_huc_restriction(wopcm->guc.size,
 huc_fw_size);
if (size)
goto err;
@@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,
  
  err:

DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
- guc_wopcm_size / 1024,
+ wopcm->guc.size / 1024,
  size / 1024);
  
  	return -E2BIG;

  }
  
+static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)

+{
+   struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+   u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
+   u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
+   u32 ctx_rsvd = context_reserved_size(i915);
+   int err;
+
+   err = check_huc_fw_fits(wopcm, huc_fw_size);
+   if (err)
+   return 

Re: [Intel-gfx] [CI 1/2] drm/i915/guc: Fix null pointer dereference when GuC FW is not available

[Yaodong Li]

On 03/23/2018 11:26 AM, Michal Wajdeczko wrote:
On Fri, 23 Mar 2018 19:03:47 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:



On 03/23/2018 05:27 AM, Michal Wajdeczko wrote:
On Fri, 23 Mar 2018 13:07:15 +0100, Sagar Arun Kamble 
<sagar.a.kam...@intel.com> wrote:





On 3/23/2018 4:53 PM, Piotr Piórkowski wrote:
If GuC firmware is not available on the system and we load i915 
with enable

GuC, then we hit this null pointer dereference issue:

Patch looks good but I have query on usage of enable_guc modparam.
enable_guc modparam will enable GuC/HuC only if firmware is available.


During modparam sanitization phase, code will only check for firmware
name, code will attempt to check if firmware file exists.
Is it better if we won't even call into upload FW once we found the 
FW fetching was failed?


If user sets to forcefully enable GuC/HuC on systems that don't 
have firmware then it is user's fault.


Sure its user's fault, but event in such case we should just gracefully
abort driver load, without any NULL pointer BUG ;)

And note, that we will hit this bug not only when user force GuC with:
    enable_guc=1 guc_firmware_path=/does/not/exist

but also when user just specify auto mode:
    enable_guc=-1

when predefined firmwares are not present (not installed or removed)

it feels like it's still user's fault even with the auto mode. why 
the user even set

the enable_guc to -1 after known the fact that the FWs are not present?
I mean should users be expected to know the fact that the auto mode
enable_guc = -1 will enable the GuC and HuC, if the guc_firmware_path 
was left

to be none?


GuC fw path will not be none, as we have some hardcoded paths in the 
code.

And we don't tell the user that after turning on 'auto' mode, he has to
do something special. Only later in case of fw fetch errors, we are just
printing error message with extra help:

"%s: Failed to fetch firmware %s (error %d)\n"
"%s: Firmware can be downloaded from %s\n"

In this case, back to my question: maybe it's a better idea to make 
things stops here
instead of continuing to call following functions such as uc_init, 
uc_init_hw -> fw_upload?


I agree we should make sure correct pointer access. it's a 
non-excusable fault:)


The thing actually frustrated me was that we failed to screen such an 
error
with the CI, but suddenly it broke things and tests with incorrect 
configurations
(enable_guc set to 1 or -1 + No FW available). so maybe we should add 
a case to

cover such a case in CI as well if we did have such test configurations.



I guess you can write such test on your own and submit patch to 
igt-dev ;)

:-)

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI 1/2] drm/i915/guc: Fix null pointer dereference when GuC FW is not available

[Yaodong Li]

On 03/23/2018 05:27 AM, Michal Wajdeczko wrote:
On Fri, 23 Mar 2018 13:07:15 +0100, Sagar Arun Kamble 
 wrote:





On 3/23/2018 4:53 PM, Piotr Piórkowski wrote:
If GuC firmware is not available on the system and we load i915 with 
enable

GuC, then we hit this null pointer dereference issue:

Patch looks good but I have query on usage of enable_guc modparam.
enable_guc modparam will enable GuC/HuC only if firmware is available.


During modparam sanitization phase, code will only check for firmware
name, code will attempt to check if firmware file exists.
Is it better if we won't even call into upload FW once we found the FW 
fetching was failed?


If user sets to forcefully enable GuC/HuC on systems that don't have 
firmware then it is user's fault.


Sure its user's fault, but event in such case we should just gracefully
abort driver load, without any NULL pointer BUG ;)

And note, that we will hit this bug not only when user force GuC with:
enable_guc=1 guc_firmware_path=/does/not/exist

but also when user just specify auto mode:
enable_guc=-1

when predefined firmwares are not present (not installed or removed)

it feels like it's still user's fault even with the auto mode. why the 
user even set

the enable_guc to -1 after known the fact that the FWs are not present?
I mean should users be expected to know the fact that the auto mode
enable_guc = -1 will enable the GuC and HuC, if the guc_firmware_path 
was left

to be none?

I agree we should make sure correct pointer access. it's a non-excusable 
fault:)


The thing actually frustrated me was that we failed to screen such an error
with the CI, but suddenly it broke things and tests with incorrect 
configurations
(enable_guc set to 1 or -1 + No FW available). so maybe we should add a 
case to

cover such a case in CI as well if we did have such test configurations.

Thanks,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li]

On 03/23/2018 07:01 AM, Michał Winiarski wrote:

On Thu, Mar 22, 2018 at 02:27:59PM -0700, Yaodong Li wrote:

On 03/22/2018 01:38 PM, Michał Winiarski wrote:

On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:

In current code, we only compare the locked WOPCM register values with the
calculated values. However, we can continue loading GuC/HuC firmware if the
locked (or partially locked) values were valid for current GuC/HuC firmware
sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so that we
won't fail the GuC/HuC firmware loading even if the locked register values
are different from the calculated ones.

Signed-off-by: Jackie Li <yaodong...@intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kam...@intel.com>
Cc: Michał Winiarski <michal.winiar...@intel.com>
Cc: John Spotswood <john.a.spotsw...@intel.com>
---
   drivers/gpu/drm/i915/intel_wopcm.c | 183 
+++--
   1 file changed, 157 insertions(+), 26 deletions(-)

[snip]


   /**
* intel_wopcm_init() - Initialize the WOPCM structure.
* @wopcm: pointer to intel_wopcm.
@@ -155,10 +202,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-   u32 ctx_rsvd = context_reserved_size(i915);
u32 guc_wopcm_base;
u32 guc_wopcm_size;
-   u32 guc_wopcm_rsvd;
int err;
GEM_BUG_ON(!wopcm->size);
@@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
return -E2BIG;
}
-   guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-  GUC_WOPCM_OFFSET_ALIGNMENT);
-   if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
- guc_wopcm_base / 1024);
-   return -E2BIG;
-   }
-
-   guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
-   guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
+   guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);

We already discussed this elsewhere, but just to have this part available
for wider audience:

huc_fw_size is unknown (there may be no huc firmware present) - which means
we're still not able to handle module reload from guc-without-huc into
guc-with-huc

Question remains whether we want to support this scenario, or whether we should
tie GuC and HuC together and refuse to operate early on if either one is not
present. We could remove a lot of code this way...

Thanks. As we discussed I think we should describe this problem in this way:
we shall not make any assumption on either guc_fw_size and huc_fw_size.

We can make assumptions on guc_fw_size - it's known in all the relevant cases
(enable_guc=1,2,3).
(unless we want to support varying guc_fw_size - more on that later).

What I meant is we cannot make any assumption one the actual size
of guc and huc fw:)

On the other handle, we do need calculate the WOPCM layout based on
both GuC and HuC FW sizes, especially when we want to support modparam
enable_guc to enable/disable HuC FW loading dynamically. That's why I
suggest
to update the FW fetch code to report the FW size no matter we turn the HuC
FW loading on or not.

We could do that, but we don't really *need* to do that.
If user doesn't want HuC, why are we reading HuC from the filesystem? (well,
because we decided we're going to base our paritioning around it).

Because of the enable_guc=1 and enable_guc=3 case and the fact
that we only can write to these registers for once + we need to know
the size to 100% make sure we do the partitioning correctly.
please refer to my patch set: 
https://patchwork.freedesktop.org/series/40541/

Other aspect of the discussion is whether we should support enable_guc
switching
from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
In another word whether we should support FW image updates
(add/delete/update to a new version) in the filesystem without expecting a
system
reboot. My point is it's unlikely we can support this since the FW sizes
would likely
change and we need reconfigure the WO register with the latest FW available
in
the FS, and I think it worth to do it to 100% make sure we won't run into
any
module loading/init errors.

It's never going to be 100%, but we can at least try to work with 99% ;)

With correct FW images, it would be 100%, isn't it?:)

The only reason why you need HuC size is because you're building the
partitioning from the bottom (starting from HuC).
We could just as well start from the top (starting from GuC).

That's right. but eith

Re: [Intel-gfx] [PATCH v3] drm/i915/guc: Fix null pointer dereference when GuC FW is not available

[Yaodong Li]

On 03/22/2018 11:17 AM, Piotr Piórkowski wrote:

If GuC firmware is not available on the system and we load i915 with enable
GuC, then we hit this null pointer dereference issue:

[   71.098873] BUG: unable to handle kernel NULL pointer dereference at 
0008
[   71.098938] IP: intel_uc_fw_upload+0x1f/0x360 [i915]
[   71.098947] PGD 0 P4D 0
[   71.098956] Oops:  [#1] PREEMPT SMP PTI
[   71.098965] Modules linked in: i915(O+) netconsole x86_pkg_temp_thermal 
intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
mei_me i2c_i801 prime_numbers mei [last unloaded: i915]
[   71.099005] CPU: 2 PID: 1167 Comm: insmod Tainted: G U  W  O 
4.16.0-rc1+ #337
[   71.099018] Hardware name: /NUC6i5SYB, BIOS SYSKLi35.86A.0065.2018.0103.1000 
01/03/2018
[   71.099077] RIP: 0010:intel_uc_fw_upload+0x1f/0x360 [i915]
[   71.099087] RSP: 0018:c9417aa0 EFLAGS: 00010282
[   71.099097] RAX:  RBX: 88084cad12f8 RCX: a03e9357
[   71.099108] RDX: 0002 RSI: a034dba0 RDI: 88084cad12f8
[   71.099118] RBP: 0002 R08: 88085344ca90 R09: 0001
[   71.099128] R10:  R11:  R12: 88084cad
[   71.099139] R13: a034dba0 R14: fff5 R15: 88084cad12b0
[   71.099151] FS:  7f7f24ae2740() GS:88085e20() 
knlGS:
[   71.099162] CS:  0010 DS:  ES:  CR0: 80050033
[   71.099171] CR2: 0008 CR3: 000855f48001 CR4: 003606e0
[   71.099182] Call Trace:
[   71.099246]  intel_uc_init_hw+0xc8/0x520 [i915]
[   71.099303]  i915_gem_init_hw+0x11f/0x2d0 [i915]
[   71.099364]  i915_gem_init+0x2b9/0x640 [i915]
[   71.099413]  i915_driver_load+0xb74/0x1110 [i915]
[   71.099462]  i915_pci_probe+0x2e/0x90 [i915]
[   71.099476]  pci_device_probe+0xa1/0x130
[   71.099488]  driver_probe_device+0x302/0x470
[   71.099502]  __driver_attach+0xb9/0xe0
[   71.099513]  ? driver_probe_device+0x470/0x470
[   71.099525]  ? driver_probe_device+0x470/0x470
[   71.099538]  bus_for_each_dev+0x64/0x90
[   71.099550]  bus_add_driver+0x164/0x260
[   71.099561]  ? 0xa04d6000
[   71.099572]  driver_register+0x57/0xc0
[   71.099582]  ? 0xa04d6000
[   71.099593]  do_one_initcall+0x3b/0x160
[   71.099606]  ? kmem_cache_alloc_trace+0x1c3/0x2a0
[   71.099621]  do_init_module+0x5b/0x1f9
[   71.099635]  load_module+0x2467/0x2a70
[   71.099654]  ? SyS_finit_module+0xbd/0xe0
[   71.099668]  SyS_finit_module+0xbd/0xe0
[   71.099682]  do_syscall_64+0x73/0x1c0
[   71.099694]  entry_SYSCALL_64_after_hwframe+0x26/0x9b
[   71.099706] RIP: 0033:0x7f7f23fb40d9
[   71.099717] RSP: 002b:7ffda7d67ed8 EFLAGS: 0246 ORIG_RAX: 
0139
[   71.099734] RAX: ffda RBX: 55f96e2a8870 RCX: 7f7f23fb40d9
[   71.099748] RDX:  RSI: 55f96e2a8260 RDI: 0003
[   71.099763] RBP: 55f96e2a8260 R08:  R09: 7ffda7d68088
[   71.099777] R10: 0003 R11: 0246 R12: 
[   71.099791] R13: 55f96e2a8830 R14:  R15: 55f96e2a8260
[   71.099810] Code: 00 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 41 54 
49 89 f5 55 53 48 c7 c1 57 93 3e a0 48 8b 47 10 48 89 fb 4c 8b 07 <48> 8b 68 08 
8b 47 28 85 c0 74 15 83 f8 01 48 c7 c1 5b 93 3e a0
[   71.14] RIP: intel_uc_fw_upload+0x1f/0x360 [i915] RSP: c9417aa0
[   71.100020] CR2: 0008
[   71.100031] ---[ end trace d8ac93c30ceff5b2 ]--

Fixes: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size 
calculation")

v2: don't assume it is always GuC FW (Michal)
v3: added a new variable to avoid exceeding the number of characters in the
line (Michal)

Signed-off-by: Piotr Piórkowski 
Reported-by: Radoslaw Szwichtenberg 
Cc: Michał Winiarski 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Joonas Lahtinen 
Cc: Jackie Li 
Cc: Radoslaw Szwichtenberg 
---
  drivers/gpu/drm/i915/intel_uc_fw.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
b/drivers/gpu/drm/i915/intel_uc_fw.c
index 30c73243f54d..486eb116015b 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -199,9 +199,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
   int (*xfer)(struct intel_uc_fw *uc_fw,
   struct i915_vma *vma))
  {
-   struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
struct i915_vma *vma;
int err;
+   u32 ggtt_pin_bias;
  
  	DRM_DEBUG_DRIVER("%s fw load %s\n",

 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
@@ -222,9 

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values

[Yaodong Li]

On 03/22/2018 01:38 PM, Michał Winiarski wrote:

On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:

In current code, we only compare the locked WOPCM register values with the
calculated values. However, we can continue loading GuC/HuC firmware if the
locked (or partially locked) values were valid for current GuC/HuC firmware
sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so that we
won't fail the GuC/HuC firmware loading even if the locked register values
are different from the calculated ones.

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Michał Winiarski 
Cc: John Spotswood 
---
  drivers/gpu/drm/i915/intel_wopcm.c | 183 +++--
  1 file changed, 157 insertions(+), 26 deletions(-)

[snip]


  /**
   * intel_wopcm_init() - Initialize the WOPCM structure.
   * @wopcm: pointer to intel_wopcm.
@@ -155,10 +202,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
u32 guc_fw_size = intel_uc_fw_get_upload_size(>guc.fw);
u32 huc_fw_size = intel_uc_fw_get_upload_size(>huc.fw);
-   u32 ctx_rsvd = context_reserved_size(i915);
u32 guc_wopcm_base;
u32 guc_wopcm_size;
-   u32 guc_wopcm_rsvd;
int err;
  
  	GEM_BUG_ON(!wopcm->size);

@@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
return -E2BIG;
}
  
-	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,

-  GUC_WOPCM_OFFSET_ALIGNMENT);
-   if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-   DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
- guc_wopcm_base / 1024);
-   return -E2BIG;
-   }
-
-   guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
-   guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
+   guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);

We already discussed this elsewhere, but just to have this part available
for wider audience:

huc_fw_size is unknown (there may be no huc firmware present) - which means
we're still not able to handle module reload from guc-without-huc into
guc-with-huc

Question remains whether we want to support this scenario, or whether we should
tie GuC and HuC together and refuse to operate early on if either one is not
present. We could remove a lot of code this way...

Thanks. As we discussed I think we should describe this problem in this way:
we shall not make any assumption on either guc_fw_size and huc_fw_size.
On the other handle, we do need calculate the WOPCM layout based on
both GuC and HuC FW sizes, especially when we want to support modparam
enable_guc to enable/disable HuC FW loading dynamically. That's why I 
suggest

to update the FW fetch code to report the FW size no matter we turn the HuC
FW loading on or not.
Other aspect of the discussion is whether we should support enable_guc 
switching

from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
In another word whether we should support FW image updates
(add/delete/update to a new version) in the filesystem without expecting 
a system
reboot. My point is it's unlikely we can support this since the FW sizes 
would likely
change and we need reconfigure the WO register with the latest FW 
available in
the FS, and I think it worth to do it to 100% make sure we won't run 
into any

module loading/init errors.



+   err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+  _wopcm_size);
+   if (err)
+   return err;
  
  	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",

 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
  
-	guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;

-   if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
-   DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
- (guc_fw_size + guc_wopcm_rsvd) / 1024,
- guc_wopcm_size / 1024);
-   return -E2BIG;
-   }
-
-   err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
-  huc_fw_size);
+   err = verify_calculated_values(i915, guc_fw_size, huc_fw_size,
+  guc_wopcm_base, guc_wopcm_size);

Ok - so we're calculating the values in init...

Most likely case, we are going to get these values ready without getting
any forcewake during the init. Then in init_hw we only need to update
these values to the register without wasting anytime on these calculation.
However, this patch 

Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix null pointer dereference when GuC FW is not available

[Yaodong Li]

On 03/22/2018 09:18 AM, Piotr Piórkowski wrote:

If GuC firmware is not available on the system and we load i915 with enable
GuC, then we hit this null pointer dereference issue:

[   71.098873] BUG: unable to handle kernel NULL pointer dereference at 
0008
[   71.098938] IP: intel_uc_fw_upload+0x1f/0x360 [i915]
[   71.098947] PGD 0 P4D 0
[   71.098956] Oops:  [#1] PREEMPT SMP PTI
[   71.098965] Modules linked in: i915(O+) netconsole x86_pkg_temp_thermal 
intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
mei_me i2c_i801 prime_numbers mei [last unloaded: i915]
[   71.099005] CPU: 2 PID: 1167 Comm: insmod Tainted: G U  W  O 
4.16.0-rc1+ #337
[   71.099018] Hardware name: /NUC6i5SYB, BIOS SYSKLi35.86A.0065.2018.0103.1000 
01/03/2018
[   71.099077] RIP: 0010:intel_uc_fw_upload+0x1f/0x360 [i915]
[   71.099087] RSP: 0018:c9417aa0 EFLAGS: 00010282
[   71.099097] RAX:  RBX: 88084cad12f8 RCX: a03e9357
[   71.099108] RDX: 0002 RSI: a034dba0 RDI: 88084cad12f8
[   71.099118] RBP: 0002 R08: 88085344ca90 R09: 0001
[   71.099128] R10:  R11:  R12: 88084cad
[   71.099139] R13: a034dba0 R14: fff5 R15: 88084cad12b0
[   71.099151] FS:  7f7f24ae2740() GS:88085e20() 
knlGS:
[   71.099162] CS:  0010 DS:  ES:  CR0: 80050033
[   71.099171] CR2: 0008 CR3: 000855f48001 CR4: 003606e0
[   71.099182] Call Trace:
[   71.099246]  intel_uc_init_hw+0xc8/0x520 [i915]
[   71.099303]  i915_gem_init_hw+0x11f/0x2d0 [i915]
[   71.099364]  i915_gem_init+0x2b9/0x640 [i915]
[   71.099413]  i915_driver_load+0xb74/0x1110 [i915]
[   71.099462]  i915_pci_probe+0x2e/0x90 [i915]
[   71.099476]  pci_device_probe+0xa1/0x130
[   71.099488]  driver_probe_device+0x302/0x470
[   71.099502]  __driver_attach+0xb9/0xe0
[   71.099513]  ? driver_probe_device+0x470/0x470
[   71.099525]  ? driver_probe_device+0x470/0x470
[   71.099538]  bus_for_each_dev+0x64/0x90
[   71.099550]  bus_add_driver+0x164/0x260
[   71.099561]  ? 0xa04d6000
[   71.099572]  driver_register+0x57/0xc0
[   71.099582]  ? 0xa04d6000
[   71.099593]  do_one_initcall+0x3b/0x160
[   71.099606]  ? kmem_cache_alloc_trace+0x1c3/0x2a0
[   71.099621]  do_init_module+0x5b/0x1f9
[   71.099635]  load_module+0x2467/0x2a70
[   71.099654]  ? SyS_finit_module+0xbd/0xe0
[   71.099668]  SyS_finit_module+0xbd/0xe0
[   71.099682]  do_syscall_64+0x73/0x1c0
[   71.099694]  entry_SYSCALL_64_after_hwframe+0x26/0x9b
[   71.099706] RIP: 0033:0x7f7f23fb40d9
[   71.099717] RSP: 002b:7ffda7d67ed8 EFLAGS: 0246 ORIG_RAX: 
0139
[   71.099734] RAX: ffda RBX: 55f96e2a8870 RCX: 7f7f23fb40d9
[   71.099748] RDX:  RSI: 55f96e2a8260 RDI: 0003
[   71.099763] RBP: 55f96e2a8260 R08:  R09: 7ffda7d68088
[   71.099777] R10: 0003 R11: 0246 R12: 
[   71.099791] R13: 55f96e2a8830 R14:  R15: 55f96e2a8260
[   71.099810] Code: 00 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 41 55 41 54 
49 89 f5 55 53 48 c7 c1 57 93 3e a0 48 8b 47 10 48 89 fb 4c 8b 07 <48> 8b 68 08 
8b 47 28 85 c0 74 15 83 f8 01 48 c7 c1 5b 93 3e a0
[   71.14] RIP: intel_uc_fw_upload+0x1f/0x360 [i915] RSP: c9417aa0
[   71.100020] CR2: 0008
[   71.100031] ---[ end trace d8ac93c30ceff5b2 ]--

Fixes: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size 
calculation")
Hmm:-[. it's due to 3c009e3c468 (drm/i915/guc: Rename guc_ggtt_offset to 
intel_guc_ggtt_offset)


Signed-off-by: Piotr Piórkowski 
Cc: Michał Winiarski 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Joonas Lahtinen 
Cc: Jackie Li 
Cc: Radoslaw Szwichtenberg 
---
  drivers/gpu/drm/i915/intel_uc_fw.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
b/drivers/gpu/drm/i915/intel_uc_fw.c
index 30c73243f54d..f143c2437c99 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -199,7 +199,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
   int (*xfer)(struct intel_uc_fw *uc_fw,
   struct i915_vma *vma))
  {
-   struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
+   struct intel_guc *guc = container_of(uc_fw, struct intel_guc, fw);

we don't know whether it's guc or huc at this point.
still need use to_i915(uc_fw->obj->base.dev)
but just move it right after:
    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
        return -ENOEXEC;

    GEM_BUG_ON(uc_fw->obj);

Re: [Intel-gfx] [PATCH v3] drm/i915: Use correct reST syntax for WOPCM and GuC kernel-doc diagrams

[Yaodong Li]

Thanks Sagar and Joonas! I probably need reconfigure my email client.

I will add these docs to i915.rst. Since we've already had a GuC chapter 
defined in i915.rst,


so I will include these doc like:

WOPCM

        WOPCM Layout doc

GuC

    

        GuC Address Space doc

Please let me know if I misunderstood anything.

Thanks,

-Jackie

On 03/21/2018 04:53 AM, Sagar Arun Kamble wrote:
Joonas suggests to include these files guc.c and wopcm.c in i915.rst 
with WOPCM being separate section from GuC.

Also ensure make htmldocs generates proper/expected documentation.

Thanks,
Sagar

On 3/15/2018 10:24 PM, Jackie Li wrote:
GuC Address Space and WOPCM Layout diagrams won't be generated 
correctly by

sphinx build if not using proper reST syntax.

This patch uses reST literal blocks to make sure GuC Address Space and
WOPCM Layout diagrams to be generated correctly, and it also corrects 
some

errors in the diagram description.

v2:
  - Fixed errors in diagram description

v3:
  - Updated GuC Address Space kernel-doc based on Michal's suggestion

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Joonas Lahtinen 
---
  drivers/gpu/drm/i915/intel_guc.c   | 56 
--

  drivers/gpu/drm/i915/intel_wopcm.c | 44 --
  2 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 3eb516e..bcbdf15 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -495,35 +495,37 @@ int intel_guc_resume(struct intel_guc *guc)
  /**
   * DOC: GuC Address Space
   *
- * The layout of GuC address space is shown as below:
+ * The layout of GuC address space is shown below:
   *
- *    +==> ++ <== GUC_GGTT_TOP
- *    ^    |    |
- *    |    |    |
- *    |    |    DRAM    |
- *    |    |   Memory   |
- *    |    |    |
- *   GuC   |    |
- * Address  +> ++ <== WOPCM Top
- *  Space   ^  |   HW contexts RSVD |
- *    | |  |    WOPCM   |
- *    | | +==> ++ <== GuC WOPCM Top
- *    |    GuC    ^    |    |
- *    |    GGTT   |    |    |
- *    |    Pin   GuC   |    GuC |
- *    |    Bias WOPCM  |   WOPCM    |
- *    | |    Size  |    |
- *    | | |    |    |
- *    v v v    |    |
- *    +=+=+==> ++ <== GuC WOPCM Base
- * |   Non-GuC WOPCM    |
- * |   (HuC/Reserved)   |
- * ++ <== WOPCM Base
+ * ::
   *
- * The lower part [0, GuC ggtt_pin_bias) is mapped to WOPCM which 
consists of
- * GuC WOPCM and WOPCM reserved for other usage (e.g.RC6 context). 
The value of
- * the GuC ggtt_pin_bias is determined by the actually GuC WOPCM 
size which is

- * set in GUC_WOPCM_SIZE register.
+ * +==> ++ <== GUC_GGTT_TOP
+ * ^    |    |
+ * |    |    |
+ * |    |    DRAM    |
+ * |    |   Memory   |
+ * |    |    |
+ *    GuC   |    |
+ *  Address  +> ++ <== WOPCM Top
+ *   Space   ^  |   HW contexts RSVD |
+ * | |  |    WOPCM   |
+ * | | +==> ++ <== GuC WOPCM Top
+ * |    GuC    ^    |    |
+ * |    GGTT   |    |    |
+ * |    Pin   GuC   |    GuC |
+ * |    Bias WOPCM  |   WOPCM    |
+ * | |    Size  |    |
+ * | | |    |    |
+ * v v v    |    |
+ * +=+=+==> ++ <== GuC WOPCM Base
+ *  |   Non-GuC WOPCM    |
+ *  |   (HuC/Reserved)   |
+ *  ++ <== WOPCM Base
+ *
+ * The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped 
to WOPCM
+ * while upper part of GuC Address Space [ggtt_pin_bias, 
GUC_GGTT_TOP) is mapped
+ * to DRAM. The value of the GuC ggtt_pin_bias is determined by 
WOPCM size and

+ * actual GuC WOPCM size.
   */
    /**
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index 4117886..74bf76f 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -11,28 +11,30 @@
   * DOC: WOPCM Layout
   *
   * The layout of the 

Re: [Intel-gfx] [PATCH v2] drm/i915: Use correct reST syntax for WOPCM and GuC kernel-doc diagrams

[Yaodong Li]

On 03/14/2018 11:54 PM, Sagar Arun Kamble wrote:
Are we required to add reference to intel_guc.c and intel_wopcm.c in 
Documentation/gpu/i915.rst?


hmm, I have the same question too:-). Can I modify the i915.rst to 
include kernel-doc from
WOPCM and GuC WOPCM related changes? or someone would decide what 
contents should be

included in i915 kernel doc?

From my point of view, I think it would make the kernel-doc more 
comprehensible (at least for the

wopcm part) if these diagrams were exported as a part of kernel-doc.

Thanks,
-Jackie



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Use correct reST syntax for WOPCM and GuC kernel-doc diagrams

[Yaodong Li]

On 03/14/2018 12:26 PM, Michal Wajdeczko wrote:
On Wed, 14 Mar 2018 19:44:43 +0100, Jackie Li  
wrote:


GuC Address Space and WOPCM Layout diagrams won't be generated 
correctly by

sphinx build if not using proper reST syntax.

This patch uses reST literal blocks to make sure GuC Address Space and
WOPCM Layout diagrams to be generated correctly, and it also corrects 
some

errors in the diagram description.

v2:
 - Fixed errors in diagram description

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/intel_guc.c   | 52 
--
 drivers/gpu/drm/i915/intel_wopcm.c | 44 
+---

 2 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 3eb516e..6a4f36e 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -495,35 +495,37 @@ int intel_guc_resume(struct intel_guc *guc)
 /**
  * DOC: GuC Address Space
  *
- * The layout of GuC address space is shown as below:
+ * The layout of GuC address space is shown below:
  *
- *    +==> ++ <== GUC_GGTT_TOP
- *    ^    |    |
- *    |    |    |
- *    |    |    DRAM    |
- *    |    |   Memory   |
- *    |    |    |
- *   GuC   |    |
- * Address  +> ++ <== WOPCM Top
- *  Space   ^  |   HW contexts RSVD |
- *    | |  |    WOPCM   |
- *    | | +==> ++ <== GuC WOPCM Top
- *    |    GuC    ^    |    |
- *    |    GGTT   |    |    |
- *    |    Pin   GuC   |    GuC |
- *    |    Bias WOPCM  |   WOPCM    |
- *    | |    Size  |    |
- *    | | |    |    |
- *    v v v    |    |
- *    +=+=+==> ++ <== GuC WOPCM Base
- * |   Non-GuC WOPCM    |
- * |   (HuC/Reserved)   |
- * ++ <== WOPCM Base
+ * ::
+ *
+ * +==> ++ <== GUC_GGTT_TOP
+ * ^    |    |
+ * |    |    |
+ * |    |    DRAM    |
+ * |    |   Memory   |
+ * |    |    |
+ *    GuC   |    |
+ *  Address  +> ++ <== WOPCM Top
+ *   Space   ^  |   HW contexts RSVD |
+ * | |  |    WOPCM   |
+ * | | +==> ++ <== GuC WOPCM Top
+ * |    GuC    ^    |    |
+ * |    GGTT   |    |    |
+ * |    Pin   GuC   |    GuC |
+ * |    Bias WOPCM  |   WOPCM    |
+ * | |    Size  |    |
+ * | | |    |    |
+ * v v v    |    |
+ * +=+=+==> ++ <== GuC WOPCM Base
+ *  |   Non-GuC WOPCM    |
+ *  |   (HuC/Reserved)   |
+ *  ++ <== WOPCM Base
  *
  * The lower part [0, GuC ggtt_pin_bias) is mapped to WOPCM which 
consists of

  * GuC WOPCM and WOPCM reserved for other usage (e.g.RC6 context). The


hmm, "lower part [...)" is little ambiguous here, as one may look for
"upper part [...)", so maybe better to be explicit:

"The lower part of GuC Address Space ie. [0, ggtt_pin_bias)
is mapped to WOPCM, while upper part of GuC Address Space ie.
[ggtt_pin_bias, GUC_GGTT_TOP) is mapped to DRAM."

Agree. it's more clear in this way. but I think we should remove "ie." 
update it to
"The lower part of GuC Address Space [0, ggtt_pin_bias) is mapped to 
WOPCM"?

value of
- * the GuC ggtt_pin_bias is determined by the actually GuC WOPCM 
size which is

- * set in GUC_WOPCM_SIZE register.
+ * the GuC ggtt_pin_bias is determined by the GuC WOPCM size which 
is set in

+ * GUC_WOPCM_SIZE register.
  */


hmm, I'm not sure that above statement is correct, compare your diagram
and calculation:

guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base;

also I would not mention registers here, as we don't read them while
calculating bias, so maybe something like this:

"The value of ggtt_pin_bias is determined by the WOPCM size and
actual GuC WOPCM base."


Thanks. Will update it.

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH v12 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

Failed to receive this mail for 5/6 patch (couldn't find it in my mailbox).
So pasted the comments from patchwork.

Quoting Jackie Li (2018-03-02 02:16:45)
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -219,3 +219,67 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  
> return 0;

>  }
> +
> +static inline int write_and_verify(struct drm_i915_private *dev_priv,
> +  i915_reg_t reg, u32 val, u32 mask,
> +  u32 locked_bit)
> +{
> +   u32 reg_val;
> +
> +   GEM_BUG_ON(val & ~mask);
> +
> +   I915_WRITE(reg, val);
> +
> +   reg_val = I915_READ(reg);

I guess I would have expected the error message here, with the wanted
value vs. what was in the register.


> +err_out:
> +   DRM_ERROR("Failed to init WOPCM registers:\n");
> +   DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
> + I915_READ(DMA_GUC_WOPCM_OFFSET));
> +   DRM_ERROR("GUC_WOPCM_SIZE=%#x\n", I915_READ(GUC_WOPCM_SIZE));

As this doesn't really give information what were the computed write
values. But if you see this is most useful for debuggin, this is;

Thanks for the comments!

This is for debugging. and the consideration was to print both the
offset and size register values for any reg update errors,  the
calculated values were printed as debug messages during wopcm
partitioning (wopcm_init)

Regards,
-Jackie

Reviewed-by: Joonas Lahtinen 



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v12 1/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Yaodong Li]

On 03/02/2018 12:04 AM, Sagar Arun Kamble wrote:


Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Reviewed-by: Sagar Arun Kamble  (v8)
Reviewed-by: Joonas Lahtinen  (v9)
Signed-off-by: Jackie Li 

I think maintainers will prefer as:
either

Cc:
Cc:
S-o-b:
R-b:

or

S-o-b:
Cc:
Cc:
R-b:
Thanks Sagar! I will include these changes along with other 
comments/suggestion

from Joonas (if any:-))

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v12 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Yaodong Li]

On 03/02/2018 12:04 AM, Sagar Arun Kamble wrote:

 (GUC_WOPCM_RESERVED + GEN9_GUC_FW_RESERVED)

+
+/**
+ * intel_wopcm_init_early() - Early initialization of the WOPCM.
+ * @wopcm: pointer to intel_wopcm.
+ *
+ * Setup the size of WOPCM which will be used by later on WOPCM 
partitioning.

+ */
+void intel_wopcm_init_early(struct intel_wopcm *wopcm)
+{
+    wopcm->size = GEN9_WOPCM_SIZE;
I am not sure if you plan to do this later but initializing it with 
value from gem_init_stolen now seems more appropriate.
I've been asked this for several times already. Yes. I have a plan, but 
just cannot switch to that plan right now.;-)


Thanks,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 03/01/2018 05:37 AM, Michal Wajdeczko wrote:
On Thu, 01 Mar 2018 02:01:39 +0100, Jackie Li  
wrote:




+
+    err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
+   dev_priv->wopcm.guc.size,


you should use wopcm-> instead dev_priv->wopcm. (same below)


+   GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
+   GUC_WOPCM_SIZE_LOCKED);


bikeshed: we should set BASE first, then SIZE ;)

I'd like to keep the sequence it as how the existing code is doing this.
Plus It is only called once during init, and now it works perfectly.:-)



+    if (err)
+    goto err_out;
+
+    huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
+    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
+    err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
+   dev_priv->wopcm.guc.base | huc_agent, mask,
+   GUC_WOPCM_OFFSET_VALID);


as the only supported HuC scenario for us is always with
HUC_LOADING_AGENT_GUC, maybe we should always set this bit,
but only add to mask for check conditionally? otherwise we
couldn't run first without and later with HuC... just asking


I assume what you are asking is how to deal with the use case that
we first run without HuC firmware, then we unload the driver module
and come back with a HuC FW?

In this case, we need to also recalculate the GuC region and we need
also the reg values accordingly. Unfortunately, we cannot do it now once
the regs were locked.


with function description and use wopcm pointer fixed,

Reviewed-by: Michal Wajdeczko 

Thanks again for the review.:-)
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 4/6] drm/i915: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 03/01/2018 05:14 AM, Michal Wajdeczko wrote:
On Thu, 01 Mar 2018 02:01:38 +0100, Jackie Li  
wrote:



On CNL A0 and Gen9, there's a hardware restriction that requires the
available GuC WOPCM size to be larger than or equal to HuC firmware 
size.


This patch adds new verification code to ensure the available GuC WOPCM
size to be larger than or equal to HuC firmware size on both Gen9 and 
CNL

A0.

v6:
 - Extended HuC FW size check against GuC WOPCM size to all
   Gen9 and CNL A0 platforms

v7:
 - Fixed patch format issues

v8:
 - Renamed variables and functions to avoid ambiguity (Joonas)
 - Updated commit message and comments to be more comprehensive (Sagar)

v9:
 - Moved code that is not related to restriction check into a separate
   patch and updated the commit message accordingly (Sagar/Michal)
 - Avoided to call uc_get_fw_size for better layer isolation (Michal)

v10:
 - Shorten function names and reorganized size_check code to have clear
   isolation (Joonas)
 - Removed unnecessary comments (Joonas)

v11:
 - Fixed logic error in size check (Michal)

BSpec: 10875

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: John Spotswood 
Cc: Jeff McGee 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Reviewed-by: Sagar Arun Kamble  (v8)
Signed-off-by: Jackie Li 
---
 drivers/gpu/drm/i915/intel_wopcm.c | 28 +---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
b/drivers/gpu/drm/i915/intel_wopcm.c

index bb78043..b30d7ff 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -107,8 +107,26 @@ static inline int gen9_check_dword_gap(u32 
guc_wopcm_base, u32 guc_wopcm_size)

 return 0;
 }
+static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 
huc_fw_size)

+{
+    /*
+ * On Gen9 & CNL A0, hardware requires the total available GuC 
WOPCM

+ * size to be larger than or equal to HuC firmware size. Otherwise,
+ * firmware uploading would fail.
+ */
+    if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) {
+    DRM_ERROR("HuC fw(%uKiB) won't fit in GuC WOPCM(%uKiB).\n",
+  huc_fw_size / 1024,
+  (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024);


bikeshed: in earlier patches in similar error messages, you used
"HuC FW (%KiB)" and didn't provide available space. Maybe simplest
way to unify and minimize the code is to add one "failed" tag in
wopcm_init function where you can print all values used for partitioning:

failed:
DRM_ERROR("Failed to partition %uKiB WOPCM (%d)\n", 
wopcm->size/1024, err);

DRM_ERROR("HuC FW size=%uKiB\n", ...);
DRM_ERROR("GuC FW size=%uKiB\n", ...);
return err;

I will keep it as it is now to save some time since I was told to keep 
these message

at the point where the error happened.:-)

+    return -E2BIG;
+    }
+
+    return 0;
+}
+
 static inline int check_hw_restriction(struct drm_i915_private *i915,
-   u32 guc_wopcm_base, u32 guc_wopcm_size)
+   u32 guc_wopcm_base, u32 guc_wopcm_size,
+   u32 huc_fw_size)
 {
 int err = 0;
@@ -117,7 +135,10 @@ static inline int check_hw_restriction(struct 
drm_i915_private *i915,

 if (err)
 return err;
-    return 0;
+    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, 
CNL_REVID_A0))

+    err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
+
+    return err;
 }
/**
@@ -186,7 +207,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 return -E2BIG;
 }
-    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size);
+    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+   huc_fw_size);
 if (err) {
 DRM_ERROR("Failed to meet HW restriction.\n");
 return err;


but bikeshed is not critical, so

Reviewed-by: Michal Wajdeczko 

Thanks,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v11 2/6] drm/i915: Implement dynamic GuC WOPCM offset and size calculation

[Yaodong Li]

On 03/01/2018 04:56 AM, Michal Wajdeczko wrote:
On Thu, 01 Mar 2018 02:01:36 +0100, Jackie Li  
wrote:




+    if (guc_fw_size >= wopcm->size) {
+    DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.",
+  guc_fw_size / 1024);
+    return -E2BIG;
+    }
+
+    if (huc_fw_size >= wopcm->size) {
+    DRM_ERROR("HuC FW (%uKiB) is too big to fit in WOPCM.",
+  huc_fw_size / 1024);
+    return -E2BIG;
+    }
+
+    /* Hardware requires GuC WOPCM base to be 16K aligned. */
+    guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+   GUC_WOPCM_OFFSET_ALIGNMENT);
+    if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
+    DRM_ERROR("GuC WOPCM base(%uKiB) is too big.\n",
+  guc_wopcm_base / 1024);
+    return -E2BIG;
+    }


hmm, all above checks are very unlikely, and are also covered by the
next check below, so maybe we can drop them?

These checks make sure these values are in a known range. so that we
don't need to worry about wrap around issues. e.g.
guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd may
wrap around if we don't have (guc_wopcm_base + ctx_rsvd) >= wopcm->size.



+
+    guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
+    /*
+ * GuC WOPCM size must be multiple of 4K pages. We've got the 
maximum
+ * WOPCM size available for GuC. Trim the size value to 4K 
boundary.

+ */
+    guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
+
+    DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
+ guc_wopcm_base / 1024, guc_wopcm_size / 1024);
+


bikeshed: [n, m) notation suggests range n..m, so maybe better to use

DRM_DEBUG_DRIVER("Calculated GuC WOPCM: base %uKiB size %uKiB\n",
I'd like to keep it as [n, m) to suggest it's a region and to align with 
other comments

in the code.



+    /*
+ * GuC WOPCM size needs to be big enough to include all GuC 
firmware,

+ * extra 8KiB stack for GuC firmware and GUC_WOPCM_RESERVED.
+ */
+    guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+    if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
+    DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
+  (guc_fw_size + guc_wopcm_rsvd) / 1024,
+  guc_wopcm_size / 1024);


hmm, maybe to simplify calculations (and match them directly with 
diagram)

we should introduce guc_wopcm_size_min:

guc_wopcm_size_min = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED + 
guc_fw_size;

if (guc_wopcm_size_min > guc_wopcm_size) {
DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
    guc_wopcm_size_min/1024, guc_wopcm_size/1024);

As we discussed, we need find the maximum size value to meet the HW 
requirement. So maybe

I need pass this comment as well.:-)

+    return -E2BIG;
+    }
+
+    err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size);
+    if (err) {
+    DRM_ERROR("Failed to meet HW restriction.\n");
+    return err;
+    }
+
+    wopcm->guc.base = guc_wopcm_base;
+    wopcm->guc.size = guc_wopcm_size;
+
+    return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_wopcm.h 
b/drivers/gpu/drm/i915/intel_wopcm.h

new file mode 100644
index 000..39d4847
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_wopcm.h
@@ -0,0 +1,34 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2017-2018 Intel Corporation
+ */
+
+#ifndef _INTEL_WOPCM_H_
+#define _INTEL_WOPCM_H_
+
+#include 
+
+/**
+ * struct intel_wopcm - overall WOPCM info and WOPCM regions.
+ * @size: size of overall WOPCM.


bikeshed: maybe better to place this doc would be inside struct
to do not mix documentation style ?

Prefer to keep as it is now:-)



+ * @guc: GuC WOPCM Region info.
+ */
+struct intel_wopcm {
+    u32 size;
+    struct {
+    /**
+ * @base: GuC WOPCM base which is offset from WOPCM base.
+ */
+    u32 base;
+    /**
+ * @size: size of the GuC WOPCM region.
+ */
+    u32 size;
+    } guc;
+};
+
+void intel_wopcm_init_early(struct intel_wopcm *wopcm);
+int intel_wopcm_init(struct intel_wopcm *wopcm);
+
+#endif


only few bikesheds plus one suggestion, with that:

Reviewed-by: Michal Wajdeczko 

Thanks for the review.
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/13/2018 05:18 PM, Yaodong Li wrote:

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:


+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    lreg->reg_val = I915_READ(lreg->reg);
+
+    return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
u32 new_val)

+{
+    GEM_BUG_ON(!lreg->validate);
+
+    return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+    u32 reg_val = lockable_reg_read(lreg);
+
+    return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct 
lockable_reg *lreg,

+ u32 new_val)
+{
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, new_val))
+    return true;
+
+    return false;
+    }
+
+    return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, 
u32 val)

+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    /*
+ * Write-once register was locked which may happen with either 
a faulty
+ * BIOS code or driver module reloading. We should still return 
success

+ * for the write if the register was locked with a valid value.
+ */
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, val))
+    goto out;
+
+    DRM_DEBUG_DRIVER("Register %s was locked with invalid 
value\n",

+ lreg->name);
+
+    return false;
+    }
+
+    I915_WRITE(lreg->reg, val);
+
+    if (!lockable_reg_locked_and_valid(lreg, val)) {
+    DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+    return false;
+    }


As we acknowledge that there are scenarios where registers can be 
already

locked, do we really need to make our code so complex ? Maybe

int write_and_verify(struct drm_i915_private *dev_priv,
 i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);

return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}


Yes, I agree it's too complex at least for the validation part. Thanks!

My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(

Sorry. I regret:-). I think it makes since to use my complex way
to validate the values because of the rsvd bits these regs.
a comparison I915_READ(reg) != (value | locked_bit) cannot
rule out the possibility that these regs were updated by faulty
software code (e.g. BIOS) with these rsvd bit set to random
values. and it's aways better to not make any assumption to
these rsvd bits. so I will still keep using the complex
way (explicitly clear this rsvd bit before comparison) to do this.
As for the lock_bit_check->write->verify sequence v.s. write->verify
sequence I think I can prefer to first one since it would provide
comprehensive info when any error occurs. so let's keep it?:-)

+
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)

I will avoid this implicit macro.

+    .reg = rg,    \
+    .reg_val = 0,    \
+    .lock_bit = lb,    \
+    .validate = func,    \


...and macro names should be always wrapped into ()


Thanks!

Will Add them.

Regards,
-Jackie


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/14/2018 09:07 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:



On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:






+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)


True, but the only reason why it is still not fixed is that
it requires changes in too many places ...

Sure. will avoid to use implicit params then:-) Thanks.





+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.


I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.


I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?


You can do partitioning at intel_wopcm level, but program hw at intel_uc
level when you know for sure if HuC is in use. Note that there is no 
wrong

in using results from WOPCM partitioning outside of intel_wopmc.


As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in 
our current code which means
current code won't trust the fact that ggtt offset would never be 
used after uc/guc init failed.


But after failure in WOPCM partitioning, which should be done sooner than
uc init, we should abort driver load, so there shall be no access to 
ggtt.


I would like to have more discussion on intel_wopcm over 
intel_guc_wopcm. :-)
This is the reason for this valid bit (which clearly suggests the 
struct is ready to use) - I won't


I'm not sure that "valid" flag approach is correct - you should trust 
your
code path, otherwise you will have to add "valid" flags to every 
structure

and this is not a scalable solution ;)

True. I need some courage here:-). I would doubt myself after seeing 
guc_ggtt_offset
validates every offset against the wopcm->guc.top which is the only 
reason I kept using

this flag:-(

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/14/2018 09:38 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 03:26:12 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:




On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:


Also, comparing again your drawing with the spec I think it
should look like this:

   + ++ <== WOPCM top
  /|\    | HW RESERVED    |
   | +--- +
   | |    |
   | +-- ++ <== GuC WOPCM top
   |    /|\  |    |
   | |   |    |
   |    GuC  |    |
   |   WOPCM |    GuC firmware    |
   |    size |    |
 WOPCM   |   ++
  size  \|/  |    GuC reserved    |
   | +-- ++ <== GuC WOPCM base (offset from 
WOPCM base)

   | |   WOPCM RESERVED   |
   | ++
  \|/    |    HuC firmware    |
   + ++ <== WOPCM base



+
+/* GUC WOPCM offset needs to be 16KB aligned. */
+#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
+/* 16KB reserved at the beginning of GuC WOPCM. */
+#define GUC_WOPCM_RESERVED    (16 << 10)
+/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
+#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
+/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
+#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)


Hmm, I'm still not convinced that we correctly attach it to "GuC 
WOPCM".


In the result, maybe we should stop focusing on "intel_guc_wopcm" 
and define

everything as "intel_wopcm" as it was earlier suggested by Joonas.

Then we can define our struct to match diagram above as

struct intel_wopcm {
    u32 size;
    struct {
    u32 base;
    u32 size;
    } guc;
};

u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
{
    return wopcm->guc.base + wopcm->guc.size;
}

More comments about the *top*:-)

The *top* we used in driver code to verify where the non-wopcm guc 
memory
allocation starts is the offset value from guc.base to WOPCM_TOP. so 
we don't need to

add the guc.base to it.

 From GuC point of view the *top* (boundary between wopcm and 
non-wopcm memory)

is like:
+--+ <=== GuC memory end

    Non WOPCM

+--+ <=== *top* (This is actually in top of the whole 
WOPCM)

  CTX Rsvd
+--+ <=== *__top*  (wopcm->guc.size)
   WOPCM
+--+ <=== guc base (wopcm->guc.base)

actually, we had a discussion whether we should set the guc wopcm and 
non-wopcm
boundary to *__top* but the suggestion we got is to stick to the spec 
description which
says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it 
seems
we could use *__top* as the boundary:-)). and it's because of this 
reason we called the
wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm 
between
*__top* to *top* as a part of GuC address space while trying to 
allocate non-wopcm

for GuC (even though GuC won't access CTX Rsvd at allO:-)).


Ok, so we should program GUC_WOPCM_SIZE to wopcm->guc.size and then
use (wopcm->size - wopcm->guc.base) as offset to i915_vma_pin, right?

All values of size and top are relevant to guc.base (offsets from 
guc.base), and currently
we are using guc.size + hole between guc.size and ctx reserved (if any) 
+ ctx reserved size

as the offset to i915_vma_pin as it's the actual WOPCM_TOP.:-)

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/14/2018 09:24 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:





On 02/13/2018 02:59 PM, Michal Wajdeczko wrote:
Hmm, that only proves that current partitioning code is too 
complicated :)

If you look at diagram it should be possible to implement it as

current calculation tries to set the maximum available WOPCM to avoid
Gen9 limitations. that the reason I didn't use guc_fw_size + 
GUC_WOPCM_RESERVED

for size calculation.:-)


guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));

the same as current calculation.


but definitely simpler and easier to review ;)


guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))

it's sample but likely run into gen9 gaps HW restriction.:-)


feel free to add/fix it here ;)

It will likely fall into the same steps. But sure will be



reserved = context_reserved_size(i915);

if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
    return -E2BIG;

(E)



@@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct 
intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,

 guc->wopcm.size = size;
 guc->wopcm.top = top;
-    err = guc_wopcm_size_check(guc);
+    err = guc_wopcm_size_check(guc, huc_fw_size);
 if (err) {
 DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
 return err;


I'm more and more convinced that we should use "intel_wopcm" to make
partition and all these checks

These are all GuC wopcm related, it only checks guc wopcm size. so 
I am wondering whether
I am still misunderstanding anything here?since the purpose of 
these calculation and checks are
clearly all GuC related. To be more precisely GuC DMA related. The 
driver's responsibility is to
calculate the GuC DMA offset and GuC wopcm size values based on 
guc/huc fw sizes and

all these checks are all for the correctness for the GuC wopcm size.
I don't see any reason why these should be a part of global 
intel_wopcm even if we really
want to keep something like wopcm_regions for both guc/huc(though I 
still don't know what
the benefit real is to keep something like HuC wopcm region except 
for sth like debugfs output?).
even in this case, we still at least keep these as a part of GuC 
since we really need it to setup
GuC HW :- Or do you mean we should create an intel_wopcm to carry 
info such as
global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
little bit confused here:-(


struct intel_wopcm should carry only results of WOPCM partitioning 
between
all agents including GuC. There is no need to carry fw sizes as 
those are

only needed as input for calculations.

I understand the point. but what I am trying to explain is that we 
can only focus on GuC WOPCM setting since there's no
need to keep tracking other regions since they are just results of 
setting of GuC WOPCM registers


Wrong, start thinking that values used to program GuC WOPCM registers
are derived from WOPCM partitioning that was done after taking into
account HuC WOPCM size, CTX reserved WOPCM, etc.

Then you can avoid all these tricky reverse calculation that you have.


Hmm.I don't think an abstraction would be wrong or right.;-)

In this case, I don't think the abstraction of all WOPCM regions is a 
wise thing to do because
it's unnecessary to keep tracking these only results to more memory unit 
to store
redundant data, especially after we known for sure that no one would use 
these data because
they are likely to consumed by some hw components that are transparent 
to kernel
mode driver or no hw would access them at all - so why bother? this is 
the question I had and
I believe this is the main reason I was not convinced by the idea of 
partitioning and had switched
to current implementation. (Actually, I had similar thought sin my 
earlier patches:-), and I even had
a patch which already has similar implementations, so It would be very 
fast for me to switch to these

after I am convinced).

I really respect every comments and really appreciate each correction. 
and I would even appreciate

more if I was totally convinced by good ideas:-)

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:


Also, comparing again your drawing with the spec I think it
should look like this:

   + ++ <== WOPCM top
  /|\    | HW RESERVED    |
   | +--- +
   | |    |
   | +-- ++ <== GuC WOPCM top
   |    /|\  |    |
   | |   |    |
   |    GuC  |    |
   |   WOPCM |    GuC firmware    |
   |    size |    |
 WOPCM   |   ++
  size  \|/  |    GuC reserved    |
   | +-- ++ <== GuC WOPCM base (offset from 
WOPCM base)

   | |   WOPCM RESERVED   |
   | ++
  \|/    |    HuC firmware    |
   + ++ <== WOPCM base



+
+/* GUC WOPCM offset needs to be 16KB aligned. */
+#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
+/* 16KB reserved at the beginning of GuC WOPCM. */
+#define GUC_WOPCM_RESERVED    (16 << 10)
+/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
+#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
+/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
+#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)


Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".

In the result, maybe we should stop focusing on "intel_guc_wopcm" and 
define

everything as "intel_wopcm" as it was earlier suggested by Joonas.

Then we can define our struct to match diagram above as

struct intel_wopcm {
u32 size;
struct {
    u32 base;
    u32 size;
} guc;
};

u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
{
return wopcm->guc.base + wopcm->guc.size;
}

More comments about the *top*:-)

The *top* we used in driver code to verify where the non-wopcm guc memory
allocation starts is the offset value from guc.base to WOPCM_TOP. so we 
don't need to

add the guc.base to it.

From GuC point of view the *top* (boundary between wopcm and non-wopcm 
memory)

is like:
+--+ <=== GuC memory end

   Non WOPCM

+--+ <=== *top* (This is actually in top of the whole WOPCM)
 CTX Rsvd
+--+ <=== *__top*  (wopcm->guc.size)
  WOPCM
+--+ <=== guc base (wopcm->guc.base)

actually, we had a discussion whether we should set the guc wopcm and 
non-wopcm
boundary to *__top* but the suggestion we got is to stick to the spec 
description which

says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it seems
we could use *__top* as the boundary:-)). and it's because of this 
reason we called the

wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm between
*__top* to *top* as a part of GuC address space while trying to allocate 
non-wopcm

for GuC (even though GuC won't access CTX Rsvd at allO:-)).

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:


+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    lreg->reg_val = I915_READ(lreg->reg);
+
+    return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
u32 new_val)

+{
+    GEM_BUG_ON(!lreg->validate);
+
+    return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+    u32 reg_val = lockable_reg_read(lreg);
+
+    return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct lockable_reg 
*lreg,

+ u32 new_val)
+{
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, new_val))
+    return true;
+
+    return false;
+    }
+
+    return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, u32 
val)

+{
+    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+    /*
+ * Write-once register was locked which may happen with either a 
faulty
+ * BIOS code or driver module reloading. We should still return 
success

+ * for the write if the register was locked with a valid value.
+ */
+    if (lockable_reg_locked(lreg)) {
+    if (lockable_reg_validate(lreg, val))
+    goto out;
+
+    DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
+ lreg->name);
+
+    return false;
+    }
+
+    I915_WRITE(lreg->reg, val);
+
+    if (!lockable_reg_locked_and_valid(lreg, val)) {
+    DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+    return false;
+    }


As we acknowledge that there are scenarios where registers can be already
locked, do we really need to make our code so complex ? Maybe

int write_and_verify(struct drm_i915_private *dev_priv,
 i915_reg_t reg, u32 value, u32 locked_bit)
{
I915_WRITE(reg, value);

return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}


Yes, I agree it's too complex at least for the validation part. Thanks!

My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(

+
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+    .name = #rg,    \
+    .guc = guc_wopcm_to_guc(guc_wopcm),    \


btw, implicit macro params are evil...

Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)

+    .reg = rg,    \
+    .reg_val = 0,    \
+    .lock_bit = lb,    \
+    .validate = func,    \


...and macro names should be always wrapped into ()


Thanks!
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h 
b/drivers/gpu/drm/i915/intel_guc_wopcm.h

index 13fcab6..89dd44c 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -66,7 +66,8 @@ struct intel_guc;
  * @offset: GuC WOPCM offset from the WOPCM base.
  * @size: size of GuC WOPCM for GuC firmware.
  * @top: start of the non-GuC WOPCM memory.
- * @valid: whether this structure contains valid (1-valid, 
0-invalid) info.

+ * @valid: whether the values in this struct are valid.
+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.


I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.


I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?

As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in our 
current code which means
current code won't trust the fact that ggtt offset would never be used 
after uc/guc init failed.
This is the reason for this valid bit (which clearly suggests the struct 
is ready to use) - I won't
assume the ggtt_offset would never be called even if the uc/guc_init 
returned failure.



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/13/2018 02:59 PM, Michal Wajdeczko wrote:
Hmm, that only proves that current partitioning code is too 
complicated :)

If you look at diagram it should be possible to implement it as

current calculation tries to set the maximum available WOPCM to avoid
Gen9 limitations. that the reason I didn't use guc_fw_size + 
GUC_WOPCM_RESERVED

for size calculation.:-)


guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));

the same as current calculation.

guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))

it's sample but likely run into gen9 gaps HW restriction.:-)

reserved = context_reserved_size(i915);

if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
return -E2BIG;

(E)



@@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm 
*guc_wopcm, u32 guc_fw_size,

 guc->wopcm.size = size;
 guc->wopcm.top = top;
-    err = guc_wopcm_size_check(guc);
+    err = guc_wopcm_size_check(guc, huc_fw_size);
 if (err) {
 DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
 return err;


I'm more and more convinced that we should use "intel_wopcm" to make
partition and all these checks

These are all GuC wopcm related, it only checks guc wopcm size. so I 
am wondering whether
I am still misunderstanding anything here?since the purpose of these 
calculation and checks are
clearly all GuC related. To be more precisely GuC DMA related. The 
driver's responsibility is to
calculate the GuC DMA offset and GuC wopcm size values based on 
guc/huc fw sizes and

all these checks are all for the correctness for the GuC  wopcm size.
I don't see any reason why these should be a part of global 
intel_wopcm even if we really
want to keep something like wopcm_regions for both guc/huc(though I 
still don't know what
the benefit real is to keep something like HuC wopcm region except 
for sth like debugfs output?).
even in this case, we still at least keep these as a part of GuC 
since we really need it to setup
GuC HW :- Or do you mean we should create an intel_wopcm to carry 
info such as
global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
little bit confused here:-(


struct intel_wopcm should carry only results of WOPCM partitioning 
between

all agents including GuC. There is no need to carry fw sizes as those are
only needed as input for calculations.

I understand the point. but what I am trying to explain is that we can 
only focus on GuC WOPCM setting since there's no
need to keep tracking other regions since they are just results of 
setting of GuC WOPCM registers (what we are
trying to do is to figure out a window on the WOPCM for GuC, and we 
don't need to keep tracking info such as
HuC WOPCM size, CTX reserved WOPCM size because KMD driver won't use 
these info anymore).


If you do think it's clearer to have something like 
intel_uc_wopcm.h/intel_uc_wopcm.c I can sure make these changes,
but what I am saying is we won't need to keep likely unused info data in 
KMD. And we always can treat the other parts
of WOPCM as reserved for other HW use. and only take care of what KMD 
needs to do. Please let me know if you

still think we should have something like uc_wopcm. I will work it out.

Regards,
-Jackie






___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/13/2018 08:06 AM, Michal Wajdeczko wrote:

+static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,

+    u32 huc_fw_size)
+{
+    /*
+ * On Gen9 & CNL A0, hardware requires the total available GuC 
WOPCM

+ * size to be larger than or equal to HuC firmware size. Otherwise,
+ * firmware uploading would fail.
+ */
+    if (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)


What if guc_wopcm->size < GUC_WOPCM_RESERVED ?


we've already had following check before this. which had guaranteed
guc_wopcm->size >= guc_fw_size + reserved, thus,
guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED
so, guc_wopcm->size > GUC_WOPCM_RESERVED:-)

    reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
    if ((guc_fw_size + reserved) > guc_wopcm->size) {
        DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
        return -E2BIG;
    }

+    return -E2BIG;
+
+    return 0;
+}
+
 static inline int gen9_check_dword_gap(struct intel_guc_wopcm 
*guc_wopcm)

 {
 u32 guc_wopcm_start;
@@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct 
intel_guc_wopcm *guc_wopcm)

 return 0;
 }
-static inline int guc_wopcm_size_check(struct intel_guc *guc)
+static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 
huc_fw_size)

 {
 struct drm_i915_private *i915 = guc_to_i915(guc);
 struct intel_guc_wopcm *guc_wopcm = >wopcm;
+    int err = 0;
if (IS_GEN9(i915))
-    return gen9_check_dword_gap(guc_wopcm);
+    err = gen9_check_dword_gap(guc_wopcm);
-    return 0;
+    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, 
CNL_REVID_A0))

+    err = check_huc_fw_fits(guc_wopcm, huc_fw_size);


Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() 
passes ?



oops! will fix this.:-)

+
+    return err;
 }
/**
@@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm 
*guc_wopcm, u32 guc_fw_size,

 guc->wopcm.size = size;
 guc->wopcm.top = top;
-    err = guc_wopcm_size_check(guc);
+    err = guc_wopcm_size_check(guc, huc_fw_size);
 if (err) {
 DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
 return err;


I'm more and more convinced that we should use "intel_wopcm" to make
partition and all these checks

These are all GuC wopcm related, it only checks guc wopcm size. so I am 
wondering whether
I am still misunderstanding anything here?since the purpose of these 
calculation and checks are
clearly all GuC related. To be more precisely GuC DMA related. The 
driver's responsibility is to
calculate the GuC DMA offset and GuC wopcm size values based on guc/huc 
fw sizes and

all these checks are all for the correctness for the GuC  wopcm size.
I don't see any reason why these should be a part of global intel_wopcm 
even if we really
want to keep something like wopcm_regions for both guc/huc(though I 
still don't know what
the benefit real is to keep something like HuC wopcm region except for 
sth like debugfs output?).
even in this case, we still at least keep these as a part of  GuC since 
we really need it to setup
GuC HW :- Or do you mean we should create an intel_wopcm to carry info 
such as
global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
little bit confused here:-(

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:

+
+/* GUC WOPCM offset needs to be 16KB aligned. */
+#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
+/* 16KB reserved at the beginning of GuC WOPCM. */
+#define GUC_WOPCM_RESERVED    (16 << 10)
+/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
+#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
+/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
+#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)


Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".

Can you explain the reason and more about your concerns?


In the result, maybe we should stop focusing on "intel_guc_wopcm" and 
define

everything as "intel_wopcm" as it was earlier suggested by Joonas.

Then we can define our struct to match diagram above as

struct intel_wopcm {
u32 size;
struct {
    u32 base;
    u32 size;
} guc;
};


Thanks Michal! but I don't think this is quite clean design. two reason:
0) I agree there should be something like intel_wopcm to describe
    the overall wopcm info, that what I did in my v1 patch series.
    the question is whether we really need it even if we are using
    only static wopcm size value? I think it should be more clear to
    introduce intel_wopcm as  a part of a patch that supports dynamic
    wopcm size calculation.
2) the wopcm region (a.k.a partition) is definitely a concept that should
    exist at least in uc layer. if we chose not to init uc/guc, it would be
    nonsense to init these partitions/info in an intel_wopcm which 
attached to
    drm_i915_private and we have initialized a part of this struct 
(e.g. size).

    to make it clean I will insist to have the guc wopcm region (or maybe
    the other region info) kept in uc level.
As I said the purpose of this series is to enable dynamic GuC WOPCM offset
and size calculation. it's not targeting any code refactoring and only 
serves

as a new feature enabling patch. The design principle of this series was to
provide clear new feature enabling by:
1) align with current code design and implementation.
2) provide correct hardware abstraction.
3) faultlessly enabled these new features. (e.g. dynamic size/offset 
calculation)

I think this series is now in a good shape to meet all above targets.

On the other hand, I do agree there always is some room to make our current
code clearer, but what we are talking about is further code refactoring.
Actually, I've already had some ideas of this. e.g. we could have some new
abstractions such as:

struct intel_wopcm {
    u32 size;
    /*any other global wopcm info*/
}

struct wopcm_region {
    u32 reserved; // reserved size in each region
    u32 offset; // offset of each region
    u32 size; // size of each region
};

struct intel_uc {
    struct wopcm_regions regions[];
    struct intel_uc_fw fws[];
    struct intel_guc guc;
    ...
}

struct intel_guc_dma {
    u32 fw_domains;
    lockable_reg size;
    lockable_reg offset;
    u32 flags; // e.g. loading HuC.
}

guc_dma_init()
guc_dma_hw_init()
guc_dma_xfer()

struct intel_guc {
    struct intel_guc_dma guc_dma;
    /*other guc objects*/
}

This would provide better software layer abstraction. but what I learned
from the 10 versions code submission is we need make things clear enough to
move forward. The lack of uc level abstraction is probably the reason why we
felt so frustrating about this part of code.

Can we just move forward by aligning to the current code implementation?
since what we need now is enable this new features. and we definitely can
provide more code refactoring patches based on these changes later.

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/09/2018 02:47 AM, Joonas Lahtinen wrote:

+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
+static inline bool guc_wopcm_locked(struct intel_guc *guc)
+{
+   struct drm_i915_private *i915 = guc_to_i915(guc);
+   bool size_reg_locked = __reg_locked(i915, GUC_WOPCM_SIZE);
+   bool offset_reg_locked = __reg_locked(i915, DMA_GUC_WOPCM_OFFSET);
+
+   return size_reg_locked && offset_reg_locked;

Hmm, isn't this supposed to be || instead, if either of the registers is
locked, we can't really do much?

It is an issue!

The intention was letting following GEM_BUG_ON to fail the
"only one reg" locked case. However, after adding the validation
of the already locked reg values, I think we need update the logic
code a little bit (to use the locked reg if it contained the valid value).

Also, I think it makes sense to introduce some helper
functions as you mentioned below to make things clearer.
so I will rework this part of code to make it clearer and address
this issue at the same time.

Thanks & Regards,
-Jackie

+static inline void guc_wopcm_hw_update(struct intel_guc *guc)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(guc);
+   u32 offset_reg_val;
+
+   /* GuC WOPCM registers should be unlocked at this point. */
+   GEM_BUG_ON(__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+   GEM_BUG_ON(__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));
+
+   offset_reg_val = guc->wopcm.offset;
+   if (guc->wopcm.need_load_huc_fw)

"load_huc_fw" would read better here.


+   offset_reg_val |= HUC_LOADING_AGENT_GUC;
+
+   I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
+   I915_WRITE(DMA_GUC_WOPCM_OFFSET, offset_reg_val);
+
+   GEM_BUG_ON(!__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+   GEM_BUG_ON(!__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));

This could use static inline write_lockable_reg() helper with a return value.
I think we need to propagate the error all the way up the chain, as it's
not a driver programmer error if these registers are locked, instead it is
likely to be a BIOS problem.

In the helper we could also test if the value is locked but happens to
match what we were intending to write, then we could call it success and
maybe emit an INFO message.


+}
+
+static inline bool guc_wopcm_regs_valid(struct intel_guc *guc)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(guc);
+   u32 size, offset;
+   bool guc_loads_huc;
+   u32 reg_val;
+
+   reg_val = I915_READ(GUC_WOPCM_SIZE);
+   /* GuC WOPCM size should be always multiple of 4K pages. */
+   size = reg_val & PAGE_MASK;
+
+   reg_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
+   guc_loads_huc = reg_val & HUC_LOADING_AGENT_GUC;
+   offset = reg_val & GUC_WOPCM_OFFSET_MASK;
+
+   if (guc->wopcm.need_load_huc_fw && !guc_loads_huc)
+   return false;
+
+   return (size == guc->wopcm.size) && (offset == guc->wopcm.offset);
+}

This seems to have much of what I wrote above already. I'd still split
the actual writes in a helper that will report the per-register a
WARN about "mismatch between locked register value and computed value"
or so. Dumping both the computed an actual register value.


@@ -147,6 +211,8 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, 
u32 guc_fw_size,
 guc->wopcm.offset = offset;
 guc->wopcm.size = size;
 guc->wopcm.top = top;
+   /* Use GuC to load HuC firmware if HuC firmware is present. */

Comment is again more on the "what" side, so can be dropped.


+   guc->wopcm.need_load_huc_fw = huc_fw_size ? 1 : 0;

... = huc_fw_size > 0;

Will literally read as "set guc load_huc_fw if huc_fw_size is
greater than zero."


+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
+{
+   struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
+   bool locked = guc_wopcm_locked(guc);
+
+   GEM_BUG_ON(!guc->wopcm.valid);
+
+   /*
+* Bug if driver hasn't updated the HW Registers and GuC WOPCM has been
+* locked. Return directly if WOPCM was locked and we have updated
+* the registers.
+*/
+   if (locked) {
+   if (guc->wopcm.hw_updated)
+   return 0;
+
+   /*
+* Mark as updated if registers contained correct values.
+* This will happen while reloading the driver module without
+* rebooting the system.
+*/
+   if (guc_wopcm_regs_valid(guc))
+   goto out;
+
+   /* Register locked without valid values. Abort HW init. */
+   return -EINVAL;
+   }
+
+   /* Always update registers when GuC WOPCM is not locked. */
+   guc_wopcm_hw_update(guc);

This flow will become more clear with the helpers, and we will get a
splat in the kernel message with less code about which register actually
was locked and how the value mismatched.

I'm up for discussion, but to my 

Re: [Intel-gfx] [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/09/2018 11:42 AM, Michal Wajdeczko wrote:



+static inline bool __reg_locked(struct drm_i915_private *dev_priv,
+    i915_reg_t reg)
+{
+    /* Set of bit-0 means this write-once register is locked. */
+    return I915_READ(reg) & BIT(0);
+}


Hmm, I'm still not happy as not all registers supports write-once bit.
What about something more generic/robust

static inline bool
__reg_test(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 
mask, u32 value)

{
return (I915_READ(reg) & mask) == value;
}

static inline bool
__reg_is_set(struct drm_i915_private *dev_priv, i915_reg_t reg, u32 
value)

{
return __reg_test(dev_priv, reg, value, value);
}
...
#define WOPCM_OFFSET_VALID  (1<<0)
...
#define WOPCM_LOCKED    (1<<0)
...
locked = __reg_is_set(i915, GUC_WOPCM_SIZE, WOPCM_LOCKED);
locked = __reg_is_set(i915, DMA_GUC_WOPCM_OFFSET, WOPCM_OFFSET_VALID);


Feels like a bit over engineering in this way. two reasons:
0) we use this only in this file and with full control over it.
1) All GuC WOPCM related registers are all write-once registers (and 
using the same bit

    for locking status)

+    /* GuC WOPCM registers should be unlocked at this point. */
+    GEM_BUG_ON(__reg_locked(dev_priv, GUC_WOPCM_SIZE));
+    GEM_BUG_ON(__reg_locked(dev_priv, DMA_GUC_WOPCM_OFFSET));


I'm not sure that GEM_BUG_ON can be used on bits that we don't
fully control


Agreed. at least it's not a GEM Bug.


+}
+
+static inline bool guc_wopcm_regs_valid(struct intel_guc *guc)


can't we always have intel_guc_wopcm as first param ?
I don't think it's necessary for it's a static func plus we would need 
another

wopcm_to_guc() with a guc_wopcm first param. we can save some time
with no confusion to external callers in this way. so why not?



+    /* Register locked without valid values. Abort HW init. */
+    return -EINVAL;


I'm not sure that EINVAL is correct error code here ... maybe EACCES ?

hmm, EACCES (-13) always feels like no permission to do this. It's more 
like a IO error, so
maybe -EIO and go ahead to trigger GEM warning for this such an error? 
since I found following

code in uc_init_hw()
    if (GEM_WARN_ON(ret == -EIO))
        ret = -EINVAL;

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 1/7] drm/i915/guc: Move GuC WOPCM related code into separate files

[Yaodong Li]

On 02/09/2018 08:46 AM, Michal Wajdeczko wrote:

--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -0,0 +1,48 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2017-2018 Intel Corporation
+ *


Do we really want to keep legacy license text?
Note that in other file (intel_lrc_reg.h) we have only SPDX tag.

Same question here. Updated based on the comments.

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/09/2018 09:24 AM, Michal Wajdeczko wrote:

+    struct intel_guc *guc =
+    container_of(guc_wopcm, struct intel_guc, wopcm);


If you define this as "wopcm_to_guc" inline helper, then maybe
all your functions could take "wopcm" as first parameter?

First thought is this is only one time work, so wanted to keep the code
simple. The following patch added an inline for this when there are more
places needs to use wopcm_to_guc.



+    u32 reserved = guc_wopcm_context_reserved_size(guc);
+    u32 offset, size, top;
+    int err;
-    /* On BXT, the top of WOPCM is reserved for RC6 context */
-    if (IS_GEN9_LP(i915))
-    size -= BXT_GUC_WOPCM_RC6_RESERVED;
+    if (!guc_fw_size)
+    return -EINVAL;


As there are many reasons to fail, maybe it would be good idea to
add at least DRM_DEBUG_DRIVER to each failing condition?


Honestly. that's my personal preference too:-) but I am trying to catch
up with the coding style to avoid these redundancies.
I thought there are redundant because:
0) this func is called by uc_init which will print error message when
    this failed.
1) there are two types of errors: invalid parameters. or FW sizes are
 too big to fit in WOPCM. the upper layer error message could 
easily reflect

    these info by check the error code.
and more, I now do feel some of these checks are redundant and will
remove them. :-)

+
+    if (reserved >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;


Do we really need to check this every time in runtime?
I think we can enforce this as GEM_BUG_ON here or in
guc_wopcm_context_reserved_size() function.


Yes. this is redundant since we are total having control
of this value.



+/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
+#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
+
+/*
+ * GuC WOPCM starts at 144KB (GUC_WOPCM_RESERVED + 128KB reserved 
for GuC

+ * firmware loading) from GuC WOPCM offset on BXT.
+ */
+#define GEN9_GUC_WOPCM_OFFSET    (144 << 10)


Other BXT specific macro (BXT_GUC_WOPCM_RC6_CTX_RESERVED) was
defined with BXT_ prefix ... can we use common prefix?


BXT suggests this is only for Gen9 LP while Gen9 means it's for all Gen9.:-)

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/08/2018 03:31 PM, Chris Wilson wrote:

Quoting Jackie Li (2018-02-08 23:03:54)

@@ -95,7 +97,11 @@ struct intel_guc_wopcm {
 u32 offset;
 u32 size;
 u32 top;
-   u32 valid;
+
+   /* GuC WOPCM flags below. */
+   u32 valid:1;
+   u32 hw_updated:1;
+   u32 need_load_huc_fw:1;

bool need_load_huc_fw:1; etc


@@ -147,6 +211,8 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, 
u32 guc_fw_size,
 guc->wopcm.offset = offset;
 guc->wopcm.size = size;
 guc->wopcm.top = top;
+   /* Use GuC to load HuC firmware if HuC firmware is present. */
+   guc->wopcm.need_load_huc_fw = huc_fw_size ? 1 : 0;

Then the compiler will do the right thing with

guc->wopcm.need_load_huc_fw = huc_fw_size;

bools, use them ;)

Thanks Chris!

To be honest, I would still end up with *guc->wopcm.need_load_huc_fw = 
huc_fw_size ? 1 : 0*
even if I had used bool for these flags.However, my main consideration 
to use *u32* instead of
*bool* was to make it flexible to add to flags bits,  e.g. easy to 
append new flags/fields which
needs more than 1-bit (may be overthinking here). we sure can use 
u8/16/32 for such new flags,

but it's a little bit odd to have something like:
    bool flag1:1;
    u8 flag2:2;

Plus, the mysterious bool size was another reason I was reluctant to use 
bool in struct (even through

I always got size = 1 byte for a bool type :-[).

Appreciate for more insights on the use of bool.

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/07/2018 09:24 AM, Michal Wajdeczko wrote:
 int guc_wopcm_check_huc_fw_size(struct guc_wopcm *wopcm, u32 huc_size)


patch 1/6 & 2/6 are only for some code movings. but have to make it 
clean.

but I do not need this func anymore:o)


so maybe part of these code movement was unnecessary, and affected
code can be replaced directly in this patch ?

That's how I did it before!:-) but I learned my lesson that I've to keep 
every change/patch
clear enough or readers will get confused;-) Actually, I was struggling 
on this a bit, but it's clearer to have to valid bit in this
struct instead of only checking offset/size/top. And now that we have 
valid bit


But if check_hw_restrictions() fails we will return error and we should
never use this struct, so this 'valid' bit now seems more redundant.

In current code, we do verify the ggtt offset against wopcm top in our 
current code which means
current code won't trust the fact that ggtt offset would never be used 
after uc/guc init failed.
This is the reason for this valid bit - I won't assume the ggtt_offset 
would never be called even
if the uc/guc_init returned failure either, since it would be weird if 
we don't check the valid bit

in guc_ggtt_offset() but still verify the offset against the wopcm top.

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/07/2018 09:24 AM, Michal Wajdeczko wrote:
On Wed, 07 Feb 2018 05:02:00 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:





On 02/06/2018 02:56 PM, Michal Wajdeczko wrote:


+    /* Explicitly cast the return value to bool. */
+    return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);


you should avoid reusing bits defined for one register in others


it's a common bit. use BIT(0) instead?


How common?
Note that your function accepts all registers.
Btw, even for these two registers used below bit0 has different
name definitions (Locked vs Offset Valid) which we should use.


this bit suggests the w-o registers were locked, right? (I mean
no matter how we call this bit).




+    offset &= DMA_GUC_WOPCM_OFFSET_MASK;


maybe like this for easier reading:

u32 reg;

reg = I915_READ(GUC_WOPCM_SIZE);
size = reg & PAGE_MASK;

reg = I915_READ(DMA_GUC_WOPCM_OFFSET);
offset = reg & DMA_GUC_WOPCM_OFFSET_MASK;

hmm, this will clear the HUC_LOADING_AGENT_GUC.


that's expected

the values here are expected to be the same as the values
set to registers. otherwise, we need update the registers.
in the case of !USES_HUC() if we would add the change
to set HUC_LOADING_AGENT bit accordingly. we still
need such a check, right?



guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;


+
+    return guc_loads_huc && (size == guc->wopcm.size) &&


what if we run in !USES_HUC() mode?


Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. 
this bit

is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?


We can set it as before, but when we compare already initialized
registers we can ignore this bit if we're running without HuC.
It must match only if we use HuC.


I can add these changes.

+    (offset == guc->wopcm.offset);
  #define GEN9_GUC_WOPCM_OFFSET    (0x24000)
  +/* GuC WOPCM flags */
+#define INTEL_GUC_WOPCM_VALID    BIT(0)
+#define INTEL_GUC_WOPCM_HW_UPDATED    BIT(1)


maybe just

bool valid:1;
bool hw_updated:1;

I was trying to avoid bool in struct (really struggling with it 
actual size), maybe

u32 valid:1;
u32 hw_updated:1


bool:1 will work the same, try it !


I sure it's going to work:-) Just obsessed with the ideas that:
0) bool (_Bool) could be any length, but it's fine for us since we know 
the compiler and hw.
1) I cannot see any benefits for using bits definition for each flag for 
two reasons:

 a) it's won't provide any performance/code readability improvement.
 b) the struct definition would grow bigger visually and we need 
explain these are flags and
 come back to modify the struct every time we need to add more 
flags.
 One more reason:-) which may not apply here, but the beauty of use 
of flags is
     we can do flags |= (flag 1 | flag 2) once instead of flags.flag1 = 
1; flags.flag2 = 1;


So let keep it;-)? (please do let me know if I've made any nonsense 
assumptions again just like we

need explicitly cast int to bool with !! even the compiler already did so)


Regards,
-Jackie



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/06/2018 02:25 PM, Michal Wajdeczko wrote:



default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  -    /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If 
GuC is not

+    /*
+ * GuC requires the ring to be placed above GuC WOPCM top. If 
GuC is not
   * present or not in use we still need a small bias as ring 
wraparound

   * at offset 0 sometimes hangs. No idea why.
   */
  if (USES_GUC(dev_priv))
-    ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+    ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;


I know that Joonas was against using extra inline function to read
value of "guc.wopcm.top" but maybe we should discuss it again as
now we maybe using invalid/unset value since now we are not verifying
"guc->wopcm.valid" flag.

It should be fine in this case since we have early inited the value to 
the worst
case (using the size of all WOPCM). we could not use valid here since 
uc_init

has been called at this point.


+static inline int gen9_guc_wopcm_size_check(struct drm_i915_private 
*i915)


maybe it would be better to pass "wopcm" instead of "i915" ?

hmm, I think it's better to have an unified parameter list for all the 
check functions.

and it the same since we will have to get wopcm from i915 anyways.

+{
+    struct intel_guc_wopcm *wopcm = >guc.wopcm;

use variable name as guc_wopcm?

+    u32 guc_wopcm_start;
+    u32 delta;
+
+    /*
+ * GuC WOPCM size is at least 4 bytes larger than the offset 
from WOPCM


Either use 4B directly below (as you give explanation here) or move that
comment near the definition of GEN9_GUC_WOPCM_DELTA.

+ * base (GuC WOPCM offset from WOPCM base + 
GEN9_GUC_WOPCM_OFFSET) due

+ * to hardware limitation on Gen9.
+ */
+    guc_wopcm_start = wopcm->offset + GEN9_GUC_WOPCM_OFFSET;
+    if (unlikely(guc_wopcm_start > wopcm->size))
+    return -E2BIG;
+
+    delta = wopcm->size - guc_wopcm_start;
+    if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
+    return -E2BIG;
+
+    return 0;
+}
+
+static inline int guc_wopcm_check_hw_restrictions(struct intel_guc 
*guc)

+{
+    struct drm_i915_private *i915 = guc_to_i915(guc);
+
+    if (IS_GEN9(i915))
+    return gen9_guc_wopcm_size_check(i915);


please use function name that matches dispatcher function

guc_wopcm_check_hw_restrictions()
gen9_guc_wopcm_check_hw_restrictions()

or

guc_wopcm_size_check()
gen9_guc_wopcm_size_check()


+
+    return 0;
+}
+
+/**
+ * intel_guc_wopcm_init() - Initialize the GuC WOPCM..

remove extra "."

   * @guc: intel guc.
+ * @guc_fw_size: size of GuC firmware.
+ * @huc_fw_size: size of HuC firmware.
   *
- * Get the platform specific GuC WOPCM size.
+ * Calculate the GuC WOPCM offset and size based on GuC and HuC 
firmware sizes.
+ * This function will to set the GuC WOPCM size to the size of 
maximum WOPCM

remove "to"
+ * available for GuC. This function will also enforce platform 
dependent
+ * hardware restrictions on GuC WOPCM offset and size. It will fail 
the GuC
+ * WOPCM init if any of these checks were failed, so that the 
following GuC

+ * firmware uploading would be aborted.
   *
- * Return: size of the GuC WOPCM.
+ * Return: 0 on success, non-zero error code on failure.
   */
-u32 intel_guc_wopcm_size(struct intel_guc *guc)


Hmm, it looks that all your changes for this function from patch 2/6
are now lost ...


patch 1/6 & 2/6 are only for some code movings. but have to make it clean.
but I do not need this func anymore:o)

+int intel_guc_wopcm_init(struct intel_guc *guc, u32 guc_fw_size,
+ u32 huc_fw_size)
  {
-    struct drm_i915_private *i915 = guc_to_i915(guc);
-    u32 size = GUC_WOPCM_TOP;
+    u32 reserved = guc_reserved_wopcm_size(guc);
+    u32 offset, size, top;
+    int err;
  -    /* On BXT, the top of WOPCM is reserved for RC6 context */
-    if (IS_GEN9_LP(i915))
-    size -= BXT_GUC_WOPCM_RC6_RESERVED;
+    if (guc->wopcm.valid)
+    return 0;


Is there a scenario when this function will be called more than one?

You're right. we need not need such a check anymore.



+
+    if (!guc_fw_size)
+    return -EINVAL;


GEM_BUG_ON ?


+
+    if (reserved >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;


GEM_BUG_ON ?

Will give the control back to uc_init.?



+
+    offset = huc_fw_size + WOPCM_RESERVED_SIZE;


What's the difference between guc_reserved_wopcm_size and 
WOPCM_RESERVED_SIZE



WOPCM_RESERVED_SIZE is the reserved size in Non-GuC WOPCM.

+    guc->wopcm.offset = offset;
+    guc->wopcm.size = size;
+    guc->wopcm.top = top;
+
+    /* Check platform specific restrictions */
+    err = guc_wopcm_check_hw_restrictions(guc);


maybe you should check HW restrictions *before* initializing the 
structure?


err = check_hw_restrictions(i915, offset, size, top);
Actually, I was struggling on this a bit, but it's clearer to have to 
valid bit in this
struct instead of only checking offset/size/top. And now that we have 
valid bit
it would be better to init the 

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/06/2018 02:56 PM, Michal Wajdeczko wrote:


+    /* Explicitly cast the return value to bool. */
+    return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);


you should avoid reusing bits defined for one register in others


it's a common bit. use BIT(0) instead?




+    offset &= DMA_GUC_WOPCM_OFFSET_MASK;


maybe like this for easier reading:

u32 reg;

reg = I915_READ(GUC_WOPCM_SIZE);
size = reg & PAGE_MASK;

reg = I915_READ(DMA_GUC_WOPCM_OFFSET);
offset = reg & DMA_GUC_WOPCM_OFFSET_MASK;

hmm, this will clear the HUC_LOADING_AGENT_GUC.

guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;


+
+    return guc_loads_huc && (size == guc->wopcm.size) &&


what if we run in !USES_HUC() mode?


Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. this bit
is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?

+    (offset == guc->wopcm.offset);
  #define GEN9_GUC_WOPCM_OFFSET    (0x24000)
  +/* GuC WOPCM flags */
+#define INTEL_GUC_WOPCM_VALID    BIT(0)
+#define INTEL_GUC_WOPCM_HW_UPDATED    BIT(1)


maybe just

bool valid:1;
bool hw_updated:1;

I was trying to avoid bool in struct (really struggling with it actual 
size), maybe

u32 valid:1;
u32 hw_updated:1

Regards,
-Jackie


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v8 1/6] drm/i915/guc: Move GuC WOPCM related code into separate files

[Yaodong Li]

On 02/06/2018 01:11 PM, Michal Wajdeczko wrote:
On Tue, 06 Feb 2018 06:15:54 +0100, Sagar Arun Kamble 
 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.h

This patch creates a better file structure by moving non-register 
related
GuC WOPCM macro definitions into a new header intel_guc_wopcm.h and 
moving

GuC WOPCM related functions to a new source file intel_guc_wopcm.c as
future patches will increase the complexity of determining the GuC 
WOPCM

offset 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 GuC

    WOPCM related code (Chris)

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Reviewed-by: Sagar Arun Kamble  (v7)
Signed-off-by: Jackie Li 

Minor change in function comment needed below as per kernel-doc format.
Otherwise, change is good.
Reviewed-by: Sagar Arun Kamble 

---
  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.h

diff --git a/drivers/gpu/drm/i915/Makefile 
b/drivers/gpu/drm/i915/Makefile

index 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 state
diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 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.h

index 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);
    #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h 
b/drivers/gpu/drm/i915/intel_guc_reg.h

index 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  0x8    /* 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    0xFEE0
diff --git 

Re: [Intel-gfx] [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/05/2018 10:39 PM, Sagar Arun Kamble wrote:

+/**
+ * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
+ * @guc: intel guc.
+ *
+ * Setup the GuC WOPCM size and offset registers with the stored 
values. It will
+ * also check the registers locking status to determine whether 
these registers

+ * are unlocked and can be updated.
+ */
+void intel_guc_wopcm_init_hw(struct intel_guc *guc)
+{
+    bool locked = guc_wopcm_locked(guc);
+
+    GEM_BUG_ON(!(guc->wopcm.flags & INTEL_GUC_WOPCM_VALID));
+
+    /*
+ * Bug if driver hasn't updated the HW Registers and GuC WOPCM 
has been

+ * locked. Return directly if WOPCM was locked and we have updated
+ * the registers.
+ */
+    if (locked) {
+    /*
+ * Mark as updated if registers contained correct values.
+ * This will happen while reloading the driver module without
+ * rebooting the system.
+ */
+    if (guc_wopcm_regs_valid(guc))
+    goto out;
+
+    GEM_BUG_ON(!(guc->wopcm.flags & INTEL_GUC_WOPCM_HW_UPDATED));
We should not go ahead with uc_init_hw in this case so returning error 
from here or

checking wopcm.flags to abort uc_init_hw is needed with debug message.

In a second thought, I think we need do more work here to check whether the
locked values are sufficient for current fw sizes. if yes then return 
success else

fail the wopcm init.
I agree we only check the status and return the control to uc_init_hw.

Thanks & Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Yaodong Li]

On 02/01/2018 02:35 PM, Chris Wilson wrote:

Quoting Yaodong Li (2018-02-01 19:47:53)

On 01/31/2018 11:38 PM, Chris Wilson wrote:

Quoting Jackie Li (2018-01-19 01:29:28)

GuC related exported functions should start with "intel_guc_"
prefix and pass intel_guc as the first parameter since its guc
related. Current guc_ggtt_offset() failed to follow this code
convention.

But it was not, and still does not operate on the guc. Is that changing?

this problem is that it's guc related and the following patches do need
to access the data from intel_guc. Do you think it's getting better
if I add a sentence like "the future patches will need to access
the intel_guc to verify the offset"?

That's the idea. You need to explain _why_ you need a particular change,
in some cases like this where it's not clear from the context of the
patch, you need to fill in the missing details for the reader. In patch
series like this where there is upfront refactoring required, remember
the reader is starting at the beginning with no idea of what's coming
next, so a bit^Wlot of foreshadowing is required in the story you tell.

Got it. Will keep that in mind. Thank you Chris:-)

Regards,
-Jackie
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 01/31/2018 11:41 PM, Chris Wilson wrote:



  - Fixed patch format issues

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 
---
+static inline bool __reg_locked(struct drm_i915_private *dev_priv,
+   i915_reg_t reg)
+{
+   return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);

Why the double cast to bool?

My thought was the code would be clearer to use bool as the return type and
have a explicit cast to bool. Are you suggesting I should use a return 
type such as *int*

and remove the double cast?

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/01/2018 12:38 AM, Sagar Arun Kamble wrote:



On 1/19/2018 6:59 AM, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM size

It would be good if you can tell about Gen9/CNL restriction briefly here.

versus HuC firmware size. With static GuC WOPCM size,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

This patch enables the dynamic calculation of the GuC WOPCM
aperture size used by GuC and HuC firmware.

we are also calculating for HuC?

Strictly speaking, we are calculating the GuC WOPCM offset based on
GuC & HuC firmware sizes. Will make it clearer. Thanks.


+    offset = huc_fw_size + WOPCM_RESERVED_SIZE;
+    if (offset >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */
+    offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
+    if ((offset + reserved) >= WOPCM_DEFAULT_SIZE)
+    return -E2BIG;
+
+    top = WOPCM_DEFAULT_SIZE - offset;
+    size = top - reserved;
+
+    /* GuC WOPCM size must be 4K aligned. */
+    size &= GUC_WOPCM_SIZE_MASK;
+

ALIGN(size, PAGE_SIZE)? Can avoid this new macro.
If you want to stay with GUC_WOPCM_SIZE_MASK, then that macro needs to 
be in guc_wopcm.h
I'd like to go with GUC_WOPCM_SIZE_MASK here since there's no sign that 
it should be related to
page size. *Align* seems not accurate here since I actually wanted to 
trim the size to 4k boundary,

will update the comments. Thanks!


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Yaodong Li]

On 01/31/2018 11:38 PM, Chris Wilson wrote:

Quoting Jackie Li (2018-01-19 01:29:28)

GuC related exported functions should start with "intel_guc_"
prefix and pass intel_guc as the first parameter since its guc
related. Current guc_ggtt_offset() failed to follow this code
convention.

But it was not, and still does not operate on the guc. Is that changing?

this problem is that it's guc related and the following patches do need
to access the data from intel_guc. Do you think it's getting better
if I add a sentence like "the future patches will need to access
the intel_guc to verify the offset"?

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 1/6] drm/i915/guc: Move GuC WOPCM related code into separate files

[Yaodong Li]

On 01/31/2018 11:37 PM, Chris Wilson wrote:

Quoting Jackie Li (2018-01-19 01:29:27)

intel_guc_reg.h should only include definition for GuC registers
and related register bits. GuC WOPCM related values should not
be defined in intel_guc_reg.h

GuC registers does not include GuC WOPCM? The code does seem to suggest
they are related ;)
  

I should say "Non-register related GuC WOPCM macros should not be
defined in intel_guc_reg.h"?

This patch creates a better file structure by moving GuC WOPCM
related definitions int to a new header intel_guc_wopcm.h
and moving GuC WOPCM related functions to a new source file
intel_guc_wopcm.c

Just needs one more sentence to sell this, perhaps "as future patches
will increase the complexity of determining the WOPCM layout".

One function per file is just crying out for LTO ;)

Thanks Chris! I will add it.

This is why I need your help and learn how to sell this:-)
Really appreciate the comments!

Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Yaodong Li]

diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c 
b/drivers/gpu/drm/i915/intel_guc_ads.c

index ac62753..7215594 100644
--- a/drivers/gpu/drm/i915/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc)
  blob->reg_state.white_list[engine->guc_id].count = 0;
  }
  -    /*
- * The GuC requires a "Golden Context" when it reinitialises
- * engines after a reset. Here we use the Render ring default
- * context, which must already exist and be pinned in the GGTT,
- * so its address won't change after we've told the GuC where
- * to find it. Note that we have to skip our header (1 page),
- * because our GuC shared data is there.
- */
-    blob->ads.golden_context_lrca =
- guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +
-    skipped_offset;

This move seems unnecessary
This move is mainly for reusing "vma" variable so that the line won't 
get too
long. Besides, this move can also make sure the assignment will get 
closer to
the code that actually uses the value from 
"blob->ads.golden_context_lrca":-)

  /*
   * The GuC expects us to exclude the portion of the context 
image that

@@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc)
  blob->ads.eng_state_size[engine->guc_id] =
  engine->context_size - skipped_size;
  -    base = guc_ggtt_offset(vma);
+    base = intel_guc_ggtt_offset(guc, vma);
  blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
  blob->ads.reg_state_buffer = base + ptr_offset(blob, 
reg_state_buffer);

  blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
  +    /*
+ * The GuC requires a "Golden Context" when it reinitialises
+ * engines after a reset. Here we use the Render ring default
+ * context, which must already exist and be pinned in the GGTT,
+ * so its address won't change after we've told the GuC where
+ * to find it. Note that we have to skip our header (1 page),
+ * because our GuC shared data is there.
+ */
+    vma = dev_priv->kernel_context->engine[RCS].state;
+    blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) +
+    skipped_offset;
+
  kunmap(page);
    return 0;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm/i915: Add a new modparam for customized ring multiplier

[Yaodong Li]

On 01/05/2018 02:15 AM, Sagar Arun Kamble wrote:



On 1/5/2018 3:22 AM, Yaodong Li wrote:


On 01/03/2018 10:10 PM, Sagar Arun Kamble wrote:
Since ring frequency programming needs consideration of both IA and 
GT frequency requests I think keeping the logic
to program the ring frequency table in driver that monitors both 
IA/GT busyness and power budgets like intel_ips
will be more appropriate. intel_ips is relying on global load 
derived from all CPUs.
I understand that power awareness and busyness based policy might be 
trickier but having that as tunable will give better flexibility.


By just looking into the current code, the way intel_ips checks gpu 
busyness cannot reflect the actual workload of GT
(e.g. gpu busy is true even if there's only one pending request), in 
this case, we shall not increase the ring freq if we
want to use a "workload monitoring" based solution. so we need a more 
accurate way to monitor the current GT workload
(e.g.  when the pending request count reaches a center tunable 
threshold??).
Yes. May be we can share the PMU data about engine busyness with 
intel_ips.
Thank you Sagar! Can you tell more about how we can get the gpu busyness 
from PMU data?


I think the solution would be to set the ring freq table to use a ring 
freq >= current ia freq (for all possible gpu freq) once we found
gpu workload is high (need to tune the threshold), and we will decrease 
the ring freq (use a 2x multiplier?) once we found the GT
workload is low. The benefit to use the intel_ips is it can tell both 
the cpu & gpu busyness. However, we do need an accurate way

to check at least the busyness of gpu for this issue.


On 1/3/2018 11:51 PM, Yaodong Li wrote:


You are thinking of plugging into intel_pstate to make it 
smarter for ia freq transitions?
Yep. This seems a correct step to give some automatic support 
instead of parameter/hardcoded multiplier.


Does this mean we should use cpufreq/intel_pstate based approach 
instead of the current modparam solution for Gen9?


Some concerns and questions about intel_pstate approach:
a) Currently, we cannot get the accurate pstate/target freq value 
from cpufreq in intel_pstate active mode since
 these values won't be exported to cpufreq layer, so if we 
won't change intel_pstate code then we only can get

 the max cpu freq of a new policy.
b) intel_pstate policy is attached to each logic cpu, which means 
we will receive policy/freq transition notification
    for each logic cpu freq change. One question is how we are 
going to decide the freq of the ring? just use the max

    cpu freq reported?
c) With the intel_pstate approach we may still run into thermal 
throttling, in this case, can a certain cooling device

    be triggered to lower the cpu freq?

Thanks and Regards,
-Jackie









___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm/i915: Add a new modparam for customized ring multiplier

[Yaodong Li]

On 01/03/2018 10:10 PM, Sagar Arun Kamble wrote:
Since ring frequency programming needs consideration of both IA and GT 
frequency requests I think keeping the logic
to program the ring frequency table in driver that monitors both IA/GT 
busyness and power budgets like intel_ips
will be more appropriate. intel_ips is relying on global load derived 
from all CPUs.
I understand that power awareness and busyness based policy might be 
trickier but having that as tunable will give better flexibility.


By just looking into the current code, the way intel_ips checks gpu 
busyness cannot reflect the actual workload of GT
(e.g. gpu busy is true even if there's only one pending request), in 
this case, we shall not increase the ring freq if we
want to use a "workload monitoring" based solution. so we need a more 
accurate way to monitor the current GT workload

(e.g.  when the pending request count reaches a center tunable threshold??).


On 1/3/2018 11:51 PM, Yaodong Li wrote:


You are thinking of plugging into intel_pstate to make it smarter 
for ia freq transitions?
Yep. This seems a correct step to give some automatic support 
instead of parameter/hardcoded multiplier.


Does this mean we should use cpufreq/intel_pstate based approach 
instead of the current modparam solution for Gen9?


Some concerns and questions about intel_pstate approach:
a) Currently, we cannot get the accurate pstate/target freq value 
from cpufreq in intel_pstate active mode since
 these values won't be exported to cpufreq layer, so if we won't 
change intel_pstate code then we only can get

 the max cpu freq of a new policy.
b) intel_pstate policy is attached to each logic cpu, which means we 
will receive policy/freq transition notification
    for each logic cpu freq change. One question is how we are going 
to decide the freq of the ring? just use the max

    cpu freq reported?
c) With the intel_pstate approach we may still run into thermal 
throttling, in this case, can a certain cooling device

    be triggered to lower the cpu freq?

Thanks and Regards,
-Jackie





___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm/i915: Add a new modparam for customized ring multiplier

[Yaodong Li]

You are thinking of plugging into intel_pstate to make it smarter for ia freq 
transitions?

Yep. This seems a correct step to give some automatic support instead of 
parameter/hardcoded multiplier.

Does this mean we should use cpufreq/intel_pstate based approach instead 
of the current modparam solution for Gen9?


Some concerns and questions about intel_pstate approach:
a) Currently, we cannot get the accurate pstate/target freq value from 
cpufreq in intel_pstate active mode since
 these values won't be exported to cpufreq layer, so if we won't 
change intel_pstate code then we only can get

 the max cpu freq of a new policy.
b) intel_pstate policy is attached to each logic cpu, which means we 
will receive policy/freq transition notification
    for each logic cpu freq change. One question is how we are going to 
decide the freq of the ring? just use the max

    cpu freq reported?
c) With the intel_pstate approach we may still run into thermal 
throttling, in this case, can a certain cooling device

    be triggered to lower the cpu freq?

Thanks and Regards,
-Jackie

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm/i915: Add a new modparam for customized ring multiplier

[Yaodong Li]

On 12/18/2017 01:47 PM, Chris Wilson wrote:

Quoting Jackie Li (2017-12-18 21:22:08)

From: Zhipeng Gong 

SKL platforms requires a higher ring multiplier when there's massive
GPU load. Current driver doesn't provide a way to override the ring
multiplier.

This patch adds a new module parameter to allow the overriding of
ring multiplier for Gen9 platforms.

So the default ring-scaling is not good enough, the first thing we do is
to try and ensure the defaults work for nearly all use cases. My
impression is that you want a nonlinear scalefactor, low power workloads
don't try and ramp up the ring frequencies as aggressively, high power
workloads try hard for higher frequencies, and then get throttled back
harder as well. How well can we autotune it? What events tells us if the
ratio is too high or too low?

Thanks Chris! the background was that we found that the performance
would drop on some SKL platforms with the default ring scaling factor.
and use of a 3x ring scaler can solve this problem.

One of the trigger of this issue is when there's massive GPU workloads
while having little/negligible CPU load - currently, we have no way to 
monitor

GPU/CPU workload in our driver. This is the reason why we use a modparam.
Do you have any suggestion about this?

Zhipeng and Dmitry may add more insights on this issue and related tuning.

Adding an untested unsafe modparam is a big no. If you want us to
support such a parameter, we at least need it not to taint the kernel
and come with some tests that prove it functions as intended. Better
still might be a I915_SETPARAM (or hooking up the write func of the
modparam) to support changing the value on the fly.

the intention of this patch was to add a way to override the
default 2x ring scaling factor so that we would be able choose
a different ring scaler for SKL platforms which were known that would
have massive GPU workload. And related changes (using a 3x scaler)
had been tested.

It would be great if we can do this on the fly.  However, we're going
to need a way to monitor the trigger events in order to change the
value dynamically.

By saying use I915_SETPARAM (or different write func), are you suggesting
to request the new ring freq directly while setting the new ring scaling 
factor?


Regards,
-Jackie

-Chris


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/15/2017 02:21 AM, Joonas Lahtinen wrote:

On Thu, 2017-12-14 at 20:55 -0800, Yaodong Li wrote:

On 12/14/2017 03:43 AM, Joonas Lahtinen wrote:

On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote:

On 12/13/2017 01:34 PM, Michal Wajdeczko wrote:

On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong...@intel.com>
wrote:


On 12/13/2017 01:11 AM, Joonas Lahtinen wrote:

On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM partition
size versus HuC firmware size. With static WOPCM partitioning,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

WOPCM being a shared feature of the hardware, it should not go under
intel_guc_ prefix.

There should be a clear division of what is specific to GuC feature
only and what is just a feature that happens to be used by GuC (and
equally can be used by HuC too).

the intel_guc_wopcm here only refers to the wopcm used by
GuC, this structure only defines the GuC related wopcm info.
(wopcm partition for GuC). We only need to set these values
(defined in this structure) to GuC registers. And this structure
should never be touched if GuC was disabled. so it should be
a part of GuC.


But note that yours intel_guc_wopcm is just one of many wopcm partitions.
I think it would be a good idea to create "intel_wopcm.c|h" and keep
all related code and data there (including verification of early setup
done by bios, wopcpm reporting, partitioning).

Then we can do rest of the programming right there or just take values
that
will be programmed individually by interested components (but former is
preferred to avoid spreading single feature code over too many places)


The KMD only needs to take care of the setup of the GuC WOPCM partition.
Other
HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus,
the GuC WOPM
partitioning is needed only when GuC is enabled and uc firmwares are
loaded correctly.
The only reason for us to have an intel_wopcm is to maintain the overall
WOPCM info
such as WOPCM size and base. However, it's not necessary since we can
reuse existing
driver code to get these info.

I'd go with Michal here, the WOPCM is its own entity in existence.
Partitioning defintely sounds like it should be intel_wopcm stuff,
which may yield intel_wopcm_partition under "guc", so then you are
still able to reference "guc->wopcm.base" where it makes sense.

And how that partition is programmed to GuC registers for it to be
used, is then stuff to go under intel_guc. And then you have another
intel_wopcm_partition for "huc".

We should avoid incorrect abstractions, just to avoid a few lines of
code. That's how the hardware features seem to exist, that's how we
should map them in the code.

Thanks for your comments. but I have some different opinions.

Agreed that wopcm exists no matter GuC is enabled or not. And we
can reuse existing code to get/verify related info we need for driver level
description of wopcm. that one reason I don't think we need intel_wopcm.

Regarding the partitioning - We need it only when GuC was enabled. In this
case, it makes sense to do it at least in uc level. Plus, from HW point
of view,
HW only relies on GuC wopcm offset and size to determine the layout
(or say partitions) of the wopcm. In this case, a good abstraction of
the HW
interface would be:
struct guc_wopcm {
  u32 offset;
  u32 size;
};
guc_wopcm_setup() - which does actual HW status check and GuC wopcm
setup.
guc_wopcm_init() - which init/verify the offset and size values
required by HW.
That's the second reason I think use of intel_guc_wopcm.c is more accurate
since it reflected the actual HW interface and could be enabled/disabled
along with GuC code.

Regarding the generic abstraction of intel_wopcm_partition for both GuC
& HuC.
I am not sure what's the benefit of such an abstraction. For two reasons:
a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any
interface
  to configure the partition for HuC, which means we even won't use
these info in
  the rest of the driver code.
b) For debugging and tracking propose, we can easily get overall layout
of WOPCM
  by just using overall wopcm description and GuC wopcm usage.


It's literally an entity called WOPCM, which is partitioned and one of
the partitions is used for GuC. I don't see how many more resons you
need for intel_wopcm prefix, struct intel_wopcm_partition abstraction
and struct intel_wopcm_partition instance for GuC?

Why would we try to make the naming scheme to imply something else,
it'll make the developer's life harder when trying to look at it. I had
to go look at the spec to make any sense of this, so let's try to avoid
that for the next developer.

Actually I started the patch with both intel_wo

Re: [Intel-gfx] [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/14/2017 03:43 AM, Joonas Lahtinen wrote:

On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote:

On 12/13/2017 01:34 PM, Michal Wajdeczko wrote:

On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong...@intel.com>
wrote:


On 12/13/2017 01:11 AM, Joonas Lahtinen wrote:

On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM partition
size versus HuC firmware size. With static WOPCM partitioning,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

WOPCM being a shared feature of the hardware, it should not go under
intel_guc_ prefix.

There should be a clear division of what is specific to GuC feature
only and what is just a feature that happens to be used by GuC (and
equally can be used by HuC too).

the intel_guc_wopcm here only refers to the wopcm used by
GuC, this structure only defines the GuC related wopcm info.
(wopcm partition for GuC). We only need to set these values
(defined in this structure) to GuC registers. And this structure
should never be touched if GuC was disabled. so it should be
a part of GuC.


But note that yours intel_guc_wopcm is just one of many wopcm partitions.
I think it would be a good idea to create "intel_wopcm.c|h" and keep
all related code and data there (including verification of early setup
done by bios, wopcpm reporting, partitioning).

Then we can do rest of the programming right there or just take values
that
will be programmed individually by interested components (but former is
preferred to avoid spreading single feature code over too many places)


The KMD only needs to take care of the setup of the GuC WOPCM partition.
Other
HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus,
the GuC WOPM
partitioning is needed only when GuC is enabled and uc firmwares are
loaded correctly.
The only reason for us to have an intel_wopcm is to maintain the overall
WOPCM info
such as WOPCM size and base. However, it's not necessary since we can
reuse existing
driver code to get these info.

I'd go with Michal here, the WOPCM is its own entity in existence.
Partitioning defintely sounds like it should be intel_wopcm stuff,
which may yield intel_wopcm_partition under "guc", so then you are
still able to reference "guc->wopcm.base" where it makes sense.

And how that partition is programmed to GuC registers for it to be
used, is then stuff to go under intel_guc. And then you have another
intel_wopcm_partition for "huc".

We should avoid incorrect abstractions, just to avoid a few lines of
code. That's how the hardware features seem to exist, that's how we
should map them in the code.

Thanks for your comments. but I have some different opinions.

Agreed that wopcm exists no matter GuC is enabled or not. And we
can reuse existing code to get/verify related info we need for driver level
description of wopcm. that one reason I don't think we need intel_wopcm.

Regarding the partitioning - We need it only when GuC was enabled. In this
case, it makes sense to do it at least in uc level. Plus, from HW point 
of view,

HW only relies on GuC wopcm offset and size to determine the layout
(or say partitions) of the wopcm. In this case, a good abstraction of 
the HW

interface would be:
  struct guc_wopcm {
    u32 offset;
    u32 size;
  };
  guc_wopcm_setup() - which does actual HW status check and GuC wopcm 
setup.
  guc_wopcm_init() - which init/verify the offset and size values 
required by HW.

That's the second reason I think use of intel_guc_wopcm.c is more accurate
since it reflected the actual HW interface and could be enabled/disabled
along with GuC code.

Regarding the generic abstraction of intel_wopcm_partition for both GuC 
& HuC.

I am not sure what's the benefit of such an abstraction. For two reasons:
a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any 
interface
    to configure the partition for HuC, which means we even won't use 
these info in

    the rest of the driver code.
b) For debugging and tracking propose, we can easily get overall layout 
of WOPCM

    by just using overall wopcm description and GuC wopcm usage.

Please do let me know if anything was wrong :-)

Regards,
-Jackie

Regards, Joonas


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/13/2017 01:34 PM, Michal Wajdeczko wrote:
On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong...@intel.com> 
wrote:



On 12/13/2017 01:11 AM, Joonas Lahtinen wrote:

On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM partition
size versus HuC firmware size. With static WOPCM partitioning,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

WOPCM being a shared feature of the hardware, it should not go under
intel_guc_ prefix.

There should be a clear division of what is specific to GuC feature
only and what is just a feature that happens to be used by GuC (and
equally can be used by HuC too).

the intel_guc_wopcm here only refers to the wopcm used by
GuC, this structure only defines the GuC related wopcm info.
(wopcm partition for GuC). We only need to set these values
(defined in this structure) to GuC registers. And this structure
should never be touched if GuC was disabled. so it should be
a part of GuC.



But note that yours intel_guc_wopcm is just one of many wopcm partitions.
I think it would be a good idea to create "intel_wopcm.c|h" and keep
all related code and data there (including verification of early setup
done by bios, wopcpm reporting, partitioning).

Then we can do rest of the programming right there or just take values 
that

will be programmed individually by interested components (but former is
preferred to avoid spreading single feature code over too many places)

The KMD only needs to take care of the setup of the GuC WOPCM partition. 
Other
HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, 
the GuC WOPM
partitioning is needed only when GuC is enabled and uc firmwares are 
loaded correctly.
The only reason for us to have an intel_wopcm is to maintain the overall 
WOPCM info
such as WOPCM size and base. However, it's not necessary since we can 
reuse existing

driver code to get these info.

Regards,
-Jackie

Michal


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/guc: Move GuC WOPCM related code into separate files

[Yaodong Li]

On 12/13/2017 12:19 AM, Joonas Lahtinen wrote:

On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:

intel_guc_reg.h should only include definition for GuC registers
and related register bits. GuC WOPCM related values should not
be defined in intel_guc_reg.h

This patch creates a better file structure by moving GuC WOPCM
related definitions int to a new header intel_guc_wopcm.h
and moving GuC WOPCM related functions to a new source file
intel_guc_wopcm.c

Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
Signed-off-by: Jackie Li 




+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -218,7 +218,7 @@ int intel_uc_init_hw(struct drm_i915_private *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));

This is a write-once register, the code needs to be refactored to
account that somebody (like an ugly BIOS) wrote it already and we have
to live with that value. Otherwise we're digging a hole for future
selves.

It's the way the current code works. I will work out a new patch to
do the code refactoring. Anyhow, this is a good catch! Thanks!


We should also verify that the write sticks as we expect.

For the verification - the following dynamic partitioning patch will
guarantee the correctness of this size value. As for the successful
register updating, I will address it within the new patch code refactoring
patch.


Regards, Joonas


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/13/2017 01:11 AM, Joonas Lahtinen wrote:

On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote:

Hardware may have specific restrictions on GuC WOPCM partition
size versus HuC firmware size. With static WOPCM partitioning,
there's no way to adjust the GuC WOPCM partition size based on
the actual HuC firmware size, so that GuC/HuC loading failure
would occur even if there was enough WOPCM space for both
GuC and HuC firmware.

WOPCM being a shared feature of the hardware, it should not go under
intel_guc_ prefix.

There should be a clear division of what is specific to GuC feature
only and what is just a feature that happens to be used by GuC (and
equally can be used by HuC too).

the intel_guc_wopcm here only refers to the wopcm used by
GuC, this structure only defines the GuC related wopcm info.
(wopcm partition for GuC). We only need to set these values
(defined in this structure) to GuC registers. And this structure
should never be touched if GuC was disabled. so it should be
a part of GuC.

Regards, Jackie


Regards, Joonas


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 4/5] drm/i915/guc: Add WOPCM partitioning support for CNL

[Yaodong Li]

On 12/08/2017 03:03 PM, Chris Wilson wrote:

Quoting Jackie Li (2017-12-08 21:41:51)

+static inline int cnl_a0_wopcm_size_check(struct drm_i915_private *i915)
+{
+   struct intel_guc_wopcm *wopcm = >guc.wopcm;
+   u32 huc_size = intel_uc_fw_get_size(>huc.fw);
+
+   /*
+* On CNL A0, hardware requires guc size to be larger than or equal to
+* HuC kernel size.
+*/

I couldn't find anything that told me that wopcm->size had to be greater
than GEN10_GUC_WOPCM_OFFSET. Either that is always true by construction,
in which case GEM_BUG_ON() here, or it may be too small in which case
add the test.
It's a known HW limitation on CNL A0. And Yes, it should be unlikely to 
happen,

but once it happened, we only want to disable GuC loading and submission.

+   if ((wopcm->size - GEN10_GUC_WOPCM_OFFSET) < huc_size)

Do) you) like) brackets)?

Will Fix it. :-) Thanks!

-Chris

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/5] drm/i915/guc: Implement dynamic WOPCM partitioning

[Yaodong Li]

On 12/08/2017 01:57 PM, Chris Wilson wrote:

Quoting Jackie Li (2017-12-08 21:41:50)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 21ce374..89ecf2c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,16 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 ctx->desc_template =
 default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  
-   /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not

-* present or not in use we still need a small bias as ring wraparound
-* at offset 0 sometimes hangs. No idea why.
+   /* GuC requires the ring to be placed above GuC WOPCM top. Since GuC
+* WOPCM won't be available until intel_uc_init_hw(), we will place
+* the context above WOPCM instead if GuC WOPCM wasn't initialized.
+* if GuC is not present or not in use we still need a small bias as
+* ring wraparound at offset 0 sometimes hangs. No idea why.

So preset it to the worstcase value in early guc init.
-Chris
Thank you very much Chris! This is very helpful. Can you also help to 
review and

comment on the rest of the patch and the other patches in this serial?
Really appreciate your help! :)

  */
 if (USES_GUC(dev_priv))
-   ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+   ctx->ggtt_offset_bias = dev_priv->guc.wopcm.valid ?
+   dev_priv->guc.wopcm.top : WOPCM_DEFAULT_SIZE;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Impletments dynamic WOPCM partitioning.

[Yaodong Li]

On 11/15/2017 09:58 AM, Chris Wilson wrote:

Quoting Jackie Li (2017-11-15 17:17:08)

Static WOPCM partitioning will lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enables the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset is  set to
HuC size + reserved WOPCM size. GuC WOPCM size is set to
total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
GuC WOPCM offset will be updated based on the size of HuC firmware
while GuC WOPCM size will be set to use all the remaining WOPCM space.

v2:
  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
  - Removed unnecessary function calls (Joonas)
  - Init GuC WOPCM partition as soon as firmware fetching is completed

Fix your indenting. Use tabs, align to brackets etc.


Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 +++--
  drivers/gpu/drm/i915/intel_guc.c|  25 ---
  drivers/gpu/drm/i915/intel_guc.h|  25 +++
  drivers/gpu/drm/i915/intel_huc.c|   2 +-
  drivers/gpu/drm/i915/intel_uc.c | 116 +++-
  drivers/gpu/drm/i915/intel_uc_fw.c  |  11 ++-
  7 files changed, 163 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 2db0406..285c310 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 ctx->desc_template =
 default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  
-   /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not

+   /* GuC requires the ring to be placed above GuC WOPCM top. if GuC is not
  * present or not in use we still need a small bias as ring wraparound
  * at offset 0 sometimes hangs. No idea why.
  */
 if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
-   ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+   ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
 else
 ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
  
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h

index bc1ae7d..567df12 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
  #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 0x8   /* 512KB */
  #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4)
  
  #define HUC_STATUS2 _MMIO(0xD3B0)

  #define   HUC_FW_VERIFIED   (1<<7)
  
  /* Defines WOPCM space available to GuC firmware */

+/* Default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE (0x1 << 20)
+/* Reserved WOPCM size 16KB */
+#define WOPCM_RESERVED_SIZE(0x4000)
+/* GUC WOPCM Offset need to be 16KB aligned */
+#define WOPCM_OFFSET_ALIGNMENT (0x4000)
+/* 8KB stack reserved for GuC FW*/
+#define GUC_WOPCM_STACK_RESERVED   (0x2000)
+/* 24KB WOPCM reserved for RC6 CTX on BXT */
+#define BXT_WOPCM_RC6_RESERVED (0x6000)
+
+#define GEN9_GUC_WOPCM_DELTA   4
+#define GEN9_GUC_WOPCM_OFFSET  (0x24000)
+
  #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   0xFEE0
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9678630..0c6bd63 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
   * This is a wrapper to create an object for use with the GuC. In order to
   * use it inside the GuC, an object needs to be pinned lifetime, so we 
allocate
   * both some backing storage and a range inside the Global GTT. We must pin
- * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
+ * it in the GGTT somewhere other than than [0, GuC WOPCM top) because that
   * range is reserved inside GuC.
   

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Impletments dynamic WOPCM partitioning.

[Yaodong Li]

On 11/16/2017 08:00 PM, Sagar Arun Kamble wrote:



On 11/17/2017 3:17 AM, Michal Wajdeczko wrote:
On Thu, 16 Nov 2017 08:34:01 +0100, Sagar Arun Kamble 
 wrote:



Typo in the subject.
GLK showing failure to load GuC with this approach on CI.

On 11/15/2017 10:47 PM, Jackie Li wrote:

Static WOPCM partitioning will lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enables the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset is  set to
HuC size + reserved WOPCM size. GuC WOPCM size is set to
total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
GuC WOPCM offset will be updated based on the size of HuC firmware
while GuC WOPCM size will be set to use all the remaining WOPCM space.

v2:
  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
  - Removed unnecessary function calls (Joonas)
  - Init GuC WOPCM partition as soon as firmware fetching is completed

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 +++--
  drivers/gpu/drm/i915/intel_guc.c    |  25 ---
  drivers/gpu/drm/i915/intel_guc.h    |  25 +++
  drivers/gpu/drm/i915/intel_huc.c    |   2 +-
  drivers/gpu/drm/i915/intel_uc.c | 116 
+++-

  drivers/gpu/drm/i915/intel_uc_fw.c  |  11 ++-
  7 files changed, 163 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c

index 2db0406..285c310 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private 
*dev_priv,

  ctx->desc_template =
  default_desc_template(dev_priv, 
dev_priv->mm.aliasing_ppgtt);
  -    /* GuC requires the ring to be placed above GUC_WOPCM_TOP. 
If GuC is not
+    /* GuC requires the ring to be placed above GuC WOPCM top. if 
GuC is not
   * present or not in use we still need a small bias as ring 
wraparound

   * at offset 0 sometimes hangs. No idea why.
   */
  if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
-    ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+    ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
  else
  ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
  diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h 
b/drivers/gpu/drm/i915/i915_guc_reg.h

index bc1ae7d..567df12 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
  #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  0x8    /* 512KB */
  #define GUC_MAX_IDLE_COUNT    _MMIO(0xC3E4)
    #define HUC_STATUS2 _MMIO(0xD3B0)
  #define   HUC_FW_VERIFIED   (1<<7)
    /* Defines WOPCM space available to GuC firmware */
+/* Default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE    (0x1 << 20)
possible to get this size from register GEN6_STOLEN_RESERVED 
(ggtt->stolen_reserved_size)

+/* Reserved WOPCM size 16KB */
+#define WOPCM_RESERVED_SIZE    (0x4000)
+/* GUC WOPCM Offset need to be 16KB aligned */
+#define WOPCM_OFFSET_ALIGNMENT    (0x4000)

prefix this with GUC_ as it is specific to GuC in WOPCM

+/* 8KB stack reserved for GuC FW*/
+#define GUC_WOPCM_STACK_RESERVED    (0x2000)
+/* 24KB WOPCM reserved for RC6 CTX on BXT */
+#define BXT_WOPCM_RC6_RESERVED    (0x6000)
+
+#define GEN9_GUC_WOPCM_DELTA    4
+#define GEN9_GUC_WOPCM_OFFSET    (0x24000)


I'm not sure that definitions unrelated to register bits shall
be defined here


+
  #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    0xFEE0
diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 9678630..0c6bd63 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private 
*dev_priv)
   * This is a wrapper to create an object for use with the GuC. In 
order to
   * use it inside the GuC, an object needs to be pinned 

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Impletments dynamic WOPCM partitioning.

[Yaodong Li]

On 11/16/2017 01:47 PM, Michal Wajdeczko wrote:
On Thu, 16 Nov 2017 08:34:01 +0100, Sagar Arun Kamble 
 wrote:



Typo in the subject.
GLK showing failure to load GuC with this approach on CI.

On 11/15/2017 10:47 PM, Jackie Li wrote:

Static WOPCM partitioning will lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enables the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset is  set to
HuC size + reserved WOPCM size. GuC WOPCM size is set to
total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case,
GuC WOPCM offset will be updated based on the size of HuC firmware
while GuC WOPCM size will be set to use all the remaining WOPCM space.

v2:
  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
  - Removed unnecessary function calls (Joonas)
  - Init GuC WOPCM partition as soon as firmware fetching is completed

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 +++--
  drivers/gpu/drm/i915/intel_guc.c    |  25 ---
  drivers/gpu/drm/i915/intel_guc.h    |  25 +++
  drivers/gpu/drm/i915/intel_huc.c    |   2 +-
  drivers/gpu/drm/i915/intel_uc.c | 116 
+++-

  drivers/gpu/drm/i915/intel_uc_fw.c  |  11 ++-
  7 files changed, 163 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c

index 2db0406..285c310 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private 
*dev_priv,

  ctx->desc_template =
  default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  -    /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If 
GuC is not
+    /* GuC requires the ring to be placed above GuC WOPCM top. if 
GuC is not
   * present or not in use we still need a small bias as ring 
wraparound

   * at offset 0 sometimes hangs. No idea why.
   */
  if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
-    ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+    ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
  else
  ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
  diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h 
b/drivers/gpu/drm/i915/i915_guc_reg.h

index bc1ae7d..567df12 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
  #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  0x8    /* 512KB */
  #define GUC_MAX_IDLE_COUNT    _MMIO(0xC3E4)
    #define HUC_STATUS2 _MMIO(0xD3B0)
  #define   HUC_FW_VERIFIED   (1<<7)
    /* Defines WOPCM space available to GuC firmware */
+/* Default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE    (0x1 << 20)
possible to get this size from register GEN6_STOLEN_RESERVED 
(ggtt->stolen_reserved_size)

+/* Reserved WOPCM size 16KB */
+#define WOPCM_RESERVED_SIZE    (0x4000)
+/* GUC WOPCM Offset need to be 16KB aligned */
+#define WOPCM_OFFSET_ALIGNMENT    (0x4000)

prefix this with GUC_ as it is specific to GuC in WOPCM

+/* 8KB stack reserved for GuC FW*/
+#define GUC_WOPCM_STACK_RESERVED    (0x2000)
+/* 24KB WOPCM reserved for RC6 CTX on BXT */
+#define BXT_WOPCM_RC6_RESERVED    (0x6000)
+
+#define GEN9_GUC_WOPCM_DELTA    4
+#define GEN9_GUC_WOPCM_OFFSET    (0x24000)


I'm not sure that definitions unrelated to register bits shall
be defined here


I was trying to align with the current implementation. since we had
put definitions such as GUC_WOPCM _TOP, BXT_GUC_WOPCM_RC6_RESERVED here.
It's better to create a header file for wopcm related definitions if we 
want to keep it absolutely

clean. I will think about it. Thanks for comments.

+
  #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    0xFEE0
diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 9678630..0c6bd63 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ 

Re: [Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Yaodong Li]

On 11/07/2017 02:52 AM, Joonas Lahtinen wrote:

On Fri, 2017-11-03 at 17:18 -0700, Jackie Li wrote:

Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 




+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private 
*dev_priv)
WARN_ON(!list_empty(_priv->contexts.list));
  }
  
+static void i915_wopcm_init(struct drm_i915_private *dev_priv)

Use "struct drm_i915_private *i915" when introducing new code that is
not fiddling with MMIOs.

Will update it.

+{
+   struct intel_wopcm_info *wopcm = _priv->wopcm;
+
+   wopcm->size = WOPCM_DEFAULT_SIZE;
+
+   DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
  static int i915_load_modeset_init(struct drm_device *dev)
  {
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
  
+	/*

+* Get the wopcm memory info.
+* NOTE: this need to be called before init FW.
+*/

Useless comments. Comments should always answer question "why?", not
"what?". And "why?" only needs answering when the code is more obscure
than this. So when the code reads clearly and you don't need comments
inside the function bodies.

Will remove it.

+   i915_wopcm_init(dev_priv);
+
intel_uc_init_fw(dev_priv);

WOPCM is the reserved memory for the uC's. Why couldn't it be inside
the *_uc_* functions? It's only relevant when you have the uCs.

They are related, but different concepts. WOPCM will be there no
matter whether we use uC or not. On the other hand, we need get
the wopcm size before fetching the FW and creating kernel context
which means intel_uc_init_fw would be the best place in uC code to
call this init function. in this case it will be called no matter 
whether uC's
active or not. I think it makes more sense to merge it into ggtt/stolen 
init.

(or would drop this call completely if possible.)

+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
   * This is a wrapper to create an object for use with the GuC. In order to
   * use it inside the GuC, an object needs to be pinned lifetime, so we 
allocate
   * both some backing storage and a range inside the Global GTT. We must pin
- * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
+ * it in the GGTT somewhere other than than [0, guc wopcm_top) because that
   * range is reserved inside GuC.
   *
   * Return:A i915_vma if successful, otherwise an ERR_PTR.
@@ -358,7 +358,8 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc 
*guc, u32 size)
goto err;
  
  	ret = i915_vma_pin(vma, 0, PAGE_SIZE,

-  PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+  PIN_GLOBAL | PIN_OFFSET_BIAS |
+  intel_guc_wopcm_top(dev_priv));

Could read just as "guc->wopcm.top"? Because we're not going to change
it runtime, we could avoid a function. It's a one-off programmable
register after all.

Good idea! The reason to put it into a function was to do an assert
to make sure wopcm partition was initialized with valid offset, size values.
However, we would never get here if wopcm partition initialization failed,
so will update the code to access the value directly.

@@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc 
*guc, u32 size)
  
  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)

  {
-   u32 wopcm_size = GUC_WOPCM_TOP;
+   struct intel_wopcm_partition *wp = _priv->wopcm_partition;
  
-	/* On BXT, the top of WOPCM is reserved for RC6 context */

-   if (IS_GEN9_LP(dev_priv))
-   wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
+   GEM_BUG_ON(!wp->guc_wopcm_size);
  
-	return wopcm_size;

+   return wp->guc_wopcm_size;
+}
+
+u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
+{
+   struct intel_wopcm_partition *wp = _priv->wopcm_partition;
+
+   GEM_BUG_ON(!dev_priv->wopcm.size);
+
+   return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
+}
+
+u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
+{
+   struct intel_wopcm_partition *wp = 

Re: [Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Yaodong Li]

On 11/06/2017 12:16 AM, Sagar Arun Kamble wrote:

Please update the subject to "Implement dynamic WOPCM partitioning"
Also, commit description should be written in present tense form.

Will update it in v2.


On 11/4/2017 5:48 AM, Jackie Li wrote:

Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.
Could you tell briefly here what is being done to wopcm offset and 
size in this patch.

Will update the description in v2.


Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Cc: Daniele Ceraolo Spurio 
Cc: John Spotswood 
Cc: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_drv.c |  15 
  drivers/gpu/drm/i915/i915_drv.h |  13 
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 -
  drivers/gpu/drm/i915/intel_guc.c    |  46 ++--
  drivers/gpu/drm/i915/intel_guc.h    |  18 +
  drivers/gpu/drm/i915/intel_huc.c    |   3 +-
  drivers/gpu/drm/i915/intel_uc.c | 128 
+++-

  drivers/gpu/drm/i915/intel_uc_fw.c  |  12 ++-
  9 files changed, 223 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c 
b/drivers/gpu/drm/i915/i915_drv.c

index e7e9e06..19509fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct 
drm_i915_private *dev_priv)

  WARN_ON(!list_empty(_priv->contexts.list));
  }
  +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
+{
+    struct intel_wopcm_info *wopcm = _priv->wopcm;
+
+    wopcm->size = WOPCM_DEFAULT_SIZE;
+
+    DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
  static int i915_load_modeset_init(struct drm_device *dev)
  {
  struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct 
drm_device *dev)

  if (ret)
  goto cleanup_irq;
  +    /*
+ * Get the wopcm memory info.
+ * NOTE: this need to be called before init FW.
+ */
+    i915_wopcm_init(dev_priv);
+
I think this call might be better if done from 
i915_driver_init_hw->i915_ggtt_probe_hw->*_gmch_probe after
gem_init_stolen as this is part of stolen memory. Might help 
performing any validation w.r.t stolen memory alloc.
I am planing the reuse the info from stolen init to create the 
description in a separate patch. Likely, I will move it
right after gem_init_stolen, or make it as a part of stolen init, or 
even drop this data structure completely.

  intel_uc_init_fw(dev_priv);
    ret = i915_gem_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h

index 72bb5b5..61cd290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
  u8 voltage_level;
  };
  +struct intel_wopcm_info {
+    u32 size;
+};
+
+struct intel_wopcm_partition {
+    u32 guc_wopcm_offset;
+    u32 guc_wopcm_size;
+    u32 guc_wopcm_top;
+};
+
*_partition can become part of *_info. This can be named as 
intel_guc_wopcm_partition and drop "guc_" prefix from variable names.
intel_wopcm_info was used for description of overall wopcm info while 
intel_wopcm_partition is about how we do the partition and it's guc
related. I agree that we can rename it to intel_guc_wopcm_partition and 
remove "guc_" prefix. but I think it makes sense keep them as
separated structures since intel_wopcm_info will be updated to reuse 
info from stolen init.

  struct drm_i915_private {
  struct drm_device drm;
  @@ -2258,6 +2268,9 @@ struct drm_i915_private {
  struct intel_huc huc;
  struct intel_guc guc;
  +    struct intel_wopcm_info wopcm;
+    struct intel_wopcm_partition wopcm_partition;
+
  struct intel_csr csr;
    struct intel_gmbus gmbus[GMBUS_NUM_PINS];
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c

index 10affb3..7347fd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private 
*dev_priv,

  ctx->desc_template =
  default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  -    /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If 
GuC is not
+    /* GuC requires the ring to be placed above guc wopcm top. If 
GuC is not
   * present or not in use we 

Re: [Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Yaodong Li]

On 11/06/2017 05:24 AM, Ville Syrjälä wrote:

On Fri, Nov 03, 2017 at 05:01:09PM -0700, Jackie Li wrote:

Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Reviewed-by: Daniele Ceraolo Spurio 
Reviewed-by: John Spotswood 
Reviewed-by: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_drv.c |  15 
  drivers/gpu/drm/i915/i915_drv.h |  13 
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 -
  drivers/gpu/drm/i915/intel_guc.c|  46 ++--
  drivers/gpu/drm/i915/intel_guc.h|  18 +
  drivers/gpu/drm/i915/intel_huc.c|   3 +-
  drivers/gpu/drm/i915/intel_uc.c | 128 +++-
  drivers/gpu/drm/i915/intel_uc_fw.c  |  12 ++-
  9 files changed, 223 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e06..19509fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private 
*dev_priv)
WARN_ON(!list_empty(_priv->contexts.list));
  }
  
+static void i915_wopcm_init(struct drm_i915_private *dev_priv)

+{
+   struct intel_wopcm_info *wopcm = _priv->wopcm;
+
+   wopcm->size = WOPCM_DEFAULT_SIZE;
+
+   DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
  static int i915_load_modeset_init(struct drm_device *dev)
  {
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
  
+	/*

+* Get the wopcm memory info.
+* NOTE: this need to be called before init FW.
+*/
+   i915_wopcm_init(dev_priv);

Is this guc wopcm somehow related to the normal wopcm? And if so
are you planning to use the wopcm information we extract from the
hardware in stolen init? If this is just related to the guc then
it would seem better to bury this in some guc code.

Thanks for the comments. Yes. I am planning to reuse these
information from stolen init to create an description of overall wopcm info.
the guc related use of wopcm was encapsulated in intel_wopcm_partition.
The reason I put the initialization code here is that we are doing
a size check against the wopcm size during intel_uc_fw_fetch (so
that we won't waste time to create a gem object for a firmware object
whose size is larger than wopcm size) and we also need to guarantee the
wopcm info are available before the creation of the kernel gem context
since we need place the gem context above wopcm when guc is active.

+
intel_uc_init_fw(dev_priv);
  
  	ret = i915_gem_init(dev_priv);


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Yaodong Li]

On 11/03/2017 05:01 PM, Jackie Li wrote:

Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.

This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.

Signed-off-by: Jackie Li 
Cc: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Sujaritha Sundaresan 
Reviewed-by: Daniele Ceraolo Spurio 
Reviewed-by: John Spotswood 
Reviewed-by: Oscar Mateo 


Sorry, these Reviewed-by should be Cc. I will resend the patch.


---
  drivers/gpu/drm/i915/i915_drv.c |  15 
  drivers/gpu/drm/i915/i915_drv.h |  13 
  drivers/gpu/drm/i915/i915_gem_context.c |   4 +-
  drivers/gpu/drm/i915/i915_guc_reg.h |  18 -
  drivers/gpu/drm/i915/intel_guc.c|  46 ++--
  drivers/gpu/drm/i915/intel_guc.h|  18 +
  drivers/gpu/drm/i915/intel_huc.c|   3 +-
  drivers/gpu/drm/i915/intel_uc.c | 128 +++-
  drivers/gpu/drm/i915/intel_uc_fw.c  |  12 ++-
  9 files changed, 223 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e06..19509fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private 
*dev_priv)
WARN_ON(!list_empty(_priv->contexts.list));
  }
  
+static void i915_wopcm_init(struct drm_i915_private *dev_priv)

+{
+   struct intel_wopcm_info *wopcm = _priv->wopcm;
+
+   wopcm->size = WOPCM_DEFAULT_SIZE;
+
+   DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
  static int i915_load_modeset_init(struct drm_device *dev)
  {
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
  
+	/*

+* Get the wopcm memory info.
+* NOTE: this need to be called before init FW.
+*/
+   i915_wopcm_init(dev_priv);
+
intel_uc_init_fw(dev_priv);
  
  	ret = i915_gem_init(dev_priv);

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72bb5b5..61cd290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
u8 voltage_level;
  };
  
+struct intel_wopcm_info {

+   u32 size;
+};
+
+struct intel_wopcm_partition {
+   u32 guc_wopcm_offset;
+   u32 guc_wopcm_size;
+   u32 guc_wopcm_top;
+};
+
  struct drm_i915_private {
struct drm_device drm;
  
@@ -2258,6 +2268,9 @@ struct drm_i915_private {

struct intel_huc huc;
struct intel_guc guc;
  
+	struct intel_wopcm_info wopcm;

+   struct intel_wopcm_partition wopcm_partition;
+
struct intel_csr csr;
  
  	struct intel_gmbus gmbus[GMBUS_NUM_PINS];

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb3..7347fd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __create_hw_context(struct drm_i915_private *dev_priv,
ctx->desc_template =
default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
  
-	/* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not

+   /* GuC requires the ring to be placed above guc wopcm top. If GuC is not
 * present or not in use we still need a small bias as ring wraparound
 * at offset 0 sometimes hangs. No idea why.
 */
if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
-   ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+   ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
else
ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
  
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h

index 35cf991..d309884 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
  #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 0x8   /* 512KB */
  #define GUC_MAX_IDLE_COUNT_MMIO(0xC3E4)
  
  #define HUC_STATUS2 _MMIO(0xD3B0)

  #define   HUC_FW_VERIFIED   (1<<7)
  
  /* Defines WOPCM space available to GuC firmware */

+/* default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE (0x1 << 20)
+/*