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

2018-04-19 Thread Yaodong Li 

On 04/19/2018 08:52 AM, Michal Wajdeczko wrote:
On Mon, 16 Apr 2018 19:43:52 +0200, Yaodong Li  
wrote:



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.


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 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

2018-04-19 Thread Michal Wajdeczko 
On Mon, 16 Apr 2018 19:43:52 +0200, Yaodong Li   
wrote:



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.


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.

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)

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

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

2018-04-16 Thread 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 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

2018-04-13 Thread Michal Wajdeczko 

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


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.

/m


   GUC_WOPCM_OFFSET_VALID);
if (err)
goto err_out;

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

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

2018-04-13 Thread John Spotswood 
On Mon, 2018-04-09 at 17:42 -0700, 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
> 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 

Reviewed-by: John Spotswood 

> ---
>  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,
>      GUC_WOPCM_OFFSET_VALID);
>   if (err)
>   goto err_out;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx