Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

2017-09-29 Thread Sujaritha 



On 09/29/2017 12:10 AM, Michal Wajdeczko wrote:
On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha 
 wrote:






-Original Message-
From: Sundaresan, Sujaritha
Sent: Thursday, September 21, 2017 11:38 AM
To: intel-gfx@lists.freedesktop.org
Cc: Sundaresan, Sujaritha ; 
Wajdeczko, Michal
; Srivatsa, Anusha 
;

Mateo Lozano, Oscar ; Ceraolo Spurio, Daniele

Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading 
module


We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission".
Whenever we need i915.enable_guc_submission=1, we also need
enable_guc_loading=1.
We also need enable_guc_loading=1 when we want to verify the HuC, 
which is
every time we have a HuC (but all platforms with HuC have a GuC and 
viceversa).


v2: Clarifying the commit message (Anusha)
v3: Unify seq_puts messages, correcting inconsistencies (Michal)
v4: Rebased

Cc: Michal Wajdeczko 
Cc: Anusha Srivatsa 
Cc: Oscar Mateo 
Cc: Daniele Ceraolo Spurio 

Signed-off-by: Sujaritha Sundaresan 
---


 snip

Will do.


diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h

index 6d7d871..bd583f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3138,11 +3138,14 @@ static inline unsigned int
i915_sg_segment_size(void)
 * command submission once loaded. But these are logically independent
 * properties, so we have separate macros to test them.
 */
-#define HAS_GUC(dev_priv)    ((dev_priv)->info.has_guc)
#define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
-#define HAS_GUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)    (HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
+#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
+#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path !=
NULL)
+#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path !=
NULL)
+
+#define NEEDS_GUC_LOADING(dev_priv) \
+    (HAS_GUC(dev_priv) && \
+    (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))


What if there is GuC and we don't want to use it?  The above 
NEEDS_GUC_LOADING is still going to load it because it is available. 
So maybe there should only be:


#define NEEDS_GUC_LOADING(dev_priv) \
(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
That is, load guc since guc submission is enabled or we need guc to 
authenticate HuC.




Note that we used HAS_GUC as prerequisite that is then followed by 
logical

operator AND what guarantees us that we will try to load Guc only if its
available. In your modified macro we will try to load Guc just due to 
user

provided enable_guc_submission modparam which may not match hw caps.

On other hand we can try to rely on earlier modparam sanitization but 
I would

rather not trust that too much and keep our macro fully independent.

Michal


Yes, this is a good point.

Thanks for the review.

Regards,

Sujaritha

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

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

2017-09-29 Thread Srivatsa, Anusha 


>-Original Message-
>From: Wajdeczko, Michal
>Sent: Friday, September 29, 2017 12:10 AM
>To: Sundaresan, Sujaritha ; intel-
>g...@lists.freedesktop.org; Srivatsa, Anusha 
>Cc: Wajdeczko, Michal ; Mateo Lozano, Oscar
>; Ceraolo Spurio, Daniele
>
>Subject: Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading
>module
>
>On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha
> wrote:
>
>>
>>
>>> -Original Message-
>>> From: Sundaresan, Sujaritha
>>> Sent: Thursday, September 21, 2017 11:38 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Sundaresan, Sujaritha ; Wajdeczko,
>>> Michal
>>> ; Srivatsa, Anusha
>>> ;
>>> Mateo Lozano, Oscar ; Ceraolo Spurio, Daniele
>>> 
>>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading
>>> module
>>>
>>> We currently have two module parameters that control GuC:
>>> "enable_guc_loading" and "enable_guc_submission".
>>> Whenever we need i915.enable_guc_submission=1, we also need
>>> enable_guc_loading=1.
>>> We also need enable_guc_loading=1 when we want to verify the HuC, which
>>> is
>>> every time we have a HuC (but all platforms with HuC have a GuC and
>>> viceversa).
>>>
>>> v2: Clarifying the commit message (Anusha)
>>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>>> v4: Rebased
>>>
>>> Cc: Michal Wajdeczko 
>>> Cc: Anusha Srivatsa 
>>> Cc: Oscar Mateo 
>>> Cc: Daniele Ceraolo Spurio 
>>>
>>> Signed-off-by: Sujaritha Sundaresan 
>>> ---
>
>  snip
>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6d7d871..bd583f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>>> i915_sg_segment_size(void)
>>>  * command submission once loaded. But these are logically independent
>>>  * properties, so we have separate macros to test them.
>>>  */
>>> -#define HAS_GUC(dev_priv)  ((dev_priv)->info.has_guc)
>>> #define HAS_GUC_CT(dev_priv)((dev_priv)->info.has_guc_ct)
>>> -#define HAS_GUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
>>> -#define HAS_GUC_SCHED(dev_priv)(HAS_GUC(dev_priv))
>>> -#define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
>>> +#define HAS_GUC(dev_priv)  ((dev_priv)->info.has_guc)
>>> +#define HAS_GUC_UCODE(dev_priv)((dev_priv)->guc.fw.path !=
>>> NULL)
>>> +#define HAS_HUC_UCODE(dev_priv)((dev_priv)->huc.fw.path !=
>>> NULL)
>>> +
>>> +#define NEEDS_GUC_LOADING(dev_priv) \
>>> +   (HAS_GUC(dev_priv) && \
>>> +   (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>>
>> What if there is GuC and we don't want to use it?  The above
>> NEEDS_GUC_LOADING is still going to load it because it is available. So
>> maybe there should only be:
>>
>> #define NEEDS_GUC_LOADING(dev_priv) \
>>  (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
>> That is, load guc since guc submission is enabled or we need guc to
>> authenticate HuC.
>>
>
>Note that we used HAS_GUC as prerequisite that is then followed by logical
>operator AND what guarantees us that we will try to load Guc only if its
>available. In your modified macro we will try to load Guc just due to user
>provided enable_guc_submission modparam which may not match hw caps.
>
>On other hand we can try to rely on earlier modparam sanitization but I
>would
>rather not trust that too much and keep our macro fully independent.

Yes, you are right.

Anusha

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

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

2017-09-29 Thread Michal Wajdeczko 
On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha  
 wrote:






-Original Message-
From: Sundaresan, Sujaritha
Sent: Thursday, September 21, 2017 11:38 AM
To: intel-gfx@lists.freedesktop.org
Cc: Sundaresan, Sujaritha ; Wajdeczko,  
Michal
; Srivatsa, Anusha  
;

Mateo Lozano, Oscar ; Ceraolo Spurio, Daniele

Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading  
module


We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission".
Whenever we need i915.enable_guc_submission=1, we also need
enable_guc_loading=1.
We also need enable_guc_loading=1 when we want to verify the HuC, which  
is
every time we have a HuC (but all platforms with HuC have a GuC and  
viceversa).


v2: Clarifying the commit message (Anusha)
v3: Unify seq_puts messages, correcting inconsistencies (Michal)
v4: Rebased

Cc: Michal Wajdeczko 
Cc: Anusha Srivatsa 
Cc: Oscar Mateo 
Cc: Daniele Ceraolo Spurio 

Signed-off-by: Sujaritha Sundaresan 
---


 snip

diff --git a/drivers/gpu/drm/i915/i915_drv.h  
b/drivers/gpu/drm/i915/i915_drv.h

index 6d7d871..bd583f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3138,11 +3138,14 @@ static inline unsigned int
i915_sg_segment_size(void)
 * command submission once loaded. But these are logically independent
 * properties, so we have separate macros to test them.
 */
-#define HAS_GUC(dev_priv)  ((dev_priv)->info.has_guc)
#define HAS_GUC_CT(dev_priv)((dev_priv)->info.has_guc_ct)
-#define HAS_GUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
+#define HAS_GUC(dev_priv)  ((dev_priv)->info.has_guc)
+#define HAS_GUC_UCODE(dev_priv)((dev_priv)->guc.fw.path !=
NULL)
+#define HAS_HUC_UCODE(dev_priv)((dev_priv)->huc.fw.path !=
NULL)
+
+#define NEEDS_GUC_LOADING(dev_priv) \
+   (HAS_GUC(dev_priv) && \
+   (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))


What if there is GuC and we don't want to use it?  The above  
NEEDS_GUC_LOADING is still going to load it because it is available. So  
maybe there should only be:


#define NEEDS_GUC_LOADING(dev_priv) \
(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
That is, load guc since guc submission is enabled or we need guc to  
authenticate HuC.




Note that we used HAS_GUC as prerequisite that is then followed by logical
operator AND what guarantees us that we will try to load Guc only if its
available. In your modified macro we will try to load Guc just due to user
provided enable_guc_submission modparam which may not match hw caps.

On other hand we can try to rely on earlier modparam sanitization but I  
would

rather not trust that too much and keep our macro fully independent.

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

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

2017-09-28 Thread Sujaritha 



On 09/28/2017 03:36 PM, Srivatsa, Anusha wrote:



-Original Message-
From: Sundaresan, Sujaritha
Sent: Thursday, September 21, 2017 11:38 AM
To: intel-gfx@lists.freedesktop.org
Cc: Sundaresan, Sujaritha ; Wajdeczko, Michal
; Srivatsa, Anusha ;
Mateo Lozano, Oscar ; Ceraolo Spurio, Daniele

Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission".
Whenever we need i915.enable_guc_submission=1, we also need
enable_guc_loading=1.
We also need enable_guc_loading=1 when we want to verify the HuC, which is
every time we have a HuC (but all platforms with HuC have a GuC and viceversa).

v2: Clarifying the commit message (Anusha)
v3: Unify seq_puts messages, correcting inconsistencies (Michal)
v4: Rebased

Cc: Michal Wajdeczko 
Cc: Anusha Srivatsa 
Cc: Oscar Mateo 
Cc: Daniele Ceraolo Spurio 

Signed-off-by: Sujaritha Sundaresan 
---
drivers/gpu/drm/i915/i915_debugfs.c | 22 +
drivers/gpu/drm/i915/i915_drv.h | 11 +++--
drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c |  2 +-
drivers/gpu/drm/i915/i915_irq.c |  2 +-
drivers/gpu/drm/i915/i915_params.c  |  5 --
drivers/gpu/drm/i915/i915_params.h  |  1 -
drivers/gpu/drm/i915/intel_guc_loader.c |  6 ++-
drivers/gpu/drm/i915/intel_uc.c | 83 ++---
9 files changed, 73 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index ca6fa6d..063fbe3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1616,7 +1616,7 @@ static int i915_fbc_status(struct seq_file *m, void
*unused)
struct drm_i915_private *dev_priv = node_to_i915(m->private);

if (!HAS_FBC(dev_priv)) {
-   seq_puts(m, "FBC unsupported on this chipset\n");
+   seq_puts(m, "not supported\n");
return 0;
}

@@ -1783,7 +1783,7 @@ static int i915_ring_freq_table(struct seq_file *m, void
*unused)
unsigned int max_gpu_freq, min_gpu_freq;

if (!HAS_LLC(dev_priv)) {
-   seq_puts(m, "unsupported on this chipset\n");
+   seq_puts(m, "not supported\n");
return 0;
}

@@ -2336,8 +2336,11 @@ static int i915_huc_load_status_info(struct seq_file
*m, void *data)
struct drm_i915_private *dev_priv = node_to_i915(m->private);
struct intel_uc_fw *huc_fw = _priv->huc.fw;

-   if (!HAS_HUC_UCODE(dev_priv))
+   if (!HAS_GUC(dev_priv)){
+   seq_puts(m, "not supported\n");
return 0;
+   }
+

seq_puts(m, "HuC firmware status:\n");
seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2369,8 +2372,11 @@
static int i915_guc_load_status_info(struct seq_file *m, void *data)
struct intel_uc_fw *guc_fw = _priv->guc.fw;
u32 tmp, i;

-   if (!HAS_GUC_UCODE(dev_priv))
+   if (!HAS_GUC(dev_priv)){
+   seq_puts(m, "not supported\n");
return 0;
+   }
+

seq_printf(m, "GuC firmware status:\n");
seq_printf(m, "\tpath: %s\n",
@@ -2465,7 +2471,7 @@ static bool check_guc_submission(struct seq_file *m)

if (!guc->execbuf_client) {
seq_printf(m, "GuC submission %s\n",
-  HAS_GUC_SCHED(dev_priv) ?
+  HAS_GUC(dev_priv) ?
   "disabled" :
   "not supported");
return false;
@@ -2654,7 +2660,7 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
bool enabled = false;

if (!HAS_PSR(dev_priv)) {
-   seq_puts(m, "PSR not supported\n");
+   seq_puts(m, "not supported\n");
return 0;
}

@@ -2807,7 +2813,7 @@ static int i915_runtime_pm_status(struct seq_file *m,
void *unused)
struct pci_dev *pdev = dev_priv->drm.pdev;

if (!HAS_RUNTIME_PM(dev_priv))
-   seq_puts(m, "Runtime power management not supported\n");
+   seq_puts(m, "not supported\n");

seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
seq_printf(m, "IRQs disabled: %s\n",
@@ -3683,7 +3689,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
mutex_unlock(>mutex);
} else {
/* DRRS not supported. Print the VBT parameter*/
-   seq_puts(m, "\tDRRS Supported : No");
+   seq_puts(m, "not supported\n");
}
seq_puts(m, "\n");
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

2017-09-28 Thread Srivatsa, Anusha 


>-Original Message-
>From: Sundaresan, Sujaritha
>Sent: Thursday, September 21, 2017 11:38 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Sundaresan, Sujaritha ; Wajdeczko, Michal
>; Srivatsa, Anusha ;
>Mateo Lozano, Oscar ; Ceraolo Spurio, Daniele
>
>Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module
>
>We currently have two module parameters that control GuC:
>"enable_guc_loading" and "enable_guc_submission".
>Whenever we need i915.enable_guc_submission=1, we also need
>enable_guc_loading=1.
>We also need enable_guc_loading=1 when we want to verify the HuC, which is
>every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
>
>v2: Clarifying the commit message (Anusha)
>v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>v4: Rebased
>
>Cc: Michal Wajdeczko 
>Cc: Anusha Srivatsa 
>Cc: Oscar Mateo 
>Cc: Daniele Ceraolo Spurio 
>
>Signed-off-by: Sujaritha Sundaresan 
>---
> drivers/gpu/drm/i915/i915_debugfs.c | 22 +
> drivers/gpu/drm/i915/i915_drv.h | 11 +++--
> drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c |  2 +-
> drivers/gpu/drm/i915/i915_irq.c |  2 +-
> drivers/gpu/drm/i915/i915_params.c  |  5 --
> drivers/gpu/drm/i915/i915_params.h  |  1 -
> drivers/gpu/drm/i915/intel_guc_loader.c |  6 ++-
> drivers/gpu/drm/i915/intel_uc.c | 83 ++---
> 9 files changed, 73 insertions(+), 61 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>b/drivers/gpu/drm/i915/i915_debugfs.c
>index ca6fa6d..063fbe3 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -1616,7 +1616,7 @@ static int i915_fbc_status(struct seq_file *m, void
>*unused)
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
>
>   if (!HAS_FBC(dev_priv)) {
>-  seq_puts(m, "FBC unsupported on this chipset\n");
>+  seq_puts(m, "not supported\n");
>   return 0;
>   }
>
>@@ -1783,7 +1783,7 @@ static int i915_ring_freq_table(struct seq_file *m, void
>*unused)
>   unsigned int max_gpu_freq, min_gpu_freq;
>
>   if (!HAS_LLC(dev_priv)) {
>-  seq_puts(m, "unsupported on this chipset\n");
>+  seq_puts(m, "not supported\n");
>   return 0;
>   }
>
>@@ -2336,8 +2336,11 @@ static int i915_huc_load_status_info(struct seq_file
>*m, void *data)
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   struct intel_uc_fw *huc_fw = _priv->huc.fw;
>
>-  if (!HAS_HUC_UCODE(dev_priv))
>+  if (!HAS_GUC(dev_priv)){
>+  seq_puts(m, "not supported\n");
>   return 0;
>+  }
>+
>
>   seq_puts(m, "HuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2369,8 +2372,11 @@
>static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   struct intel_uc_fw *guc_fw = _priv->guc.fw;
>   u32 tmp, i;
>
>-  if (!HAS_GUC_UCODE(dev_priv))
>+  if (!HAS_GUC(dev_priv)){
>+  seq_puts(m, "not supported\n");
>   return 0;
>+  }
>+
>
>   seq_printf(m, "GuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n",
>@@ -2465,7 +2471,7 @@ static bool check_guc_submission(struct seq_file *m)
>
>   if (!guc->execbuf_client) {
>   seq_printf(m, "GuC submission %s\n",
>- HAS_GUC_SCHED(dev_priv) ?
>+ HAS_GUC(dev_priv) ?
>  "disabled" :
>  "not supported");
>   return false;
>@@ -2654,7 +2660,7 @@ static int i915_edp_psr_status(struct seq_file *m, void
>*data)
>   bool enabled = false;
>
>   if (!HAS_PSR(dev_priv)) {
>-  seq_puts(m, "PSR not supported\n");
>+  seq_puts(m, "not supported\n");
>   return 0;
>   }
>
>@@ -2807,7 +2813,7 @@ static int i915_runtime_pm_status(struct seq_file *m,
>void *unused)
>   struct pci_dev *pdev = dev_priv->drm.pdev;
>
>   if (!HAS_RUNTIME_PM(dev_priv))
>-  seq_puts(m, "Runtime power management not supported\n");
>+  seq_puts(m, "not supported\n");
>
>   seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
>   seq_printf(m, "IRQs disabled: %s\n",
>@@ -3683,7 +3689,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
>   mutex_unlock(>mutex);
>   } else {
>   /* DRRS not supported. Print the VBT parameter*/
>-  seq_puts(m, "\tDRRS Supported : No");
>+  seq_puts(m, "not supported\n");
>   }
>   seq_puts(m, "\n");
> }
>diff --git