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

2018-02-14 Thread Yaodong Li 


On 02/14/2018 09:24 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li  
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 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

2018-02-14 Thread Michal Wajdeczko 
On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li   
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 ;)


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.


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


I'm looking for intel_wopcm that will do partitioning and hold results.
Then you can program GuC WOPCM in static function inside intel_uc.c using
values from intel_wopcm that we guarantee to be *always* valid at that
point.

Michal
___
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

2018-02-13 Thread 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

2018-02-13 Thread Michal Wajdeczko 
On Tue, 13 Feb 2018 22:50:53 +0100, Yaodong Li   
wrote:



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;
 }


Hmm, that only proves that current partitioning code is too complicated :)
If you look at diagram it should be possible to implement it as

guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
reserved = context_reserved_size(i915);

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

(E)


+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:-(


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.

You can still program GuC region from uc code.
___
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

2018-02-13 Thread 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 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

2018-02-13 Thread Michal Wajdeczko 

On Tue, 13 Feb 2018 00:45:51 +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 verfication 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)

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_guc_wopcm.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

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

index 9a276fe..0194266 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct  
intel_guc *guc)

return 0;
 }
+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 ?


+   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 ?


+
+   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

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

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

2018-02-12 Thread Jackie Li 
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 verfication 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)

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_guc_wopcm.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c 
b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 9a276fe..0194266 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct intel_guc 
*guc)
return 0;
 }
 
+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)
+   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);
+
+   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;
-- 
2.7.4

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