[PATCH v8 0/6] drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission

[PATCH v8 0/6] drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission

[Sujaritha Sundaresan]

The first patch simply unifies different seq_puts messages found in debugfs.
Patch 2,3 and 4 involve replacing te enable_guc_loading module. Patches 5
and 6 deal with decoupling GuC logs and ADS from submission.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Sujaritha Sundaresan (6):
  drm/i915 : Unifying seq_puts messages for feature support
  drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter
  drm/i915/guc : GEM_BUG_ON for GuC reset function
  drm/i915/guc : Updating GuC and HuC firmware select function
  drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter
  drm/i915/guc : Decouple logs and ADS from submission

 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c        |  17 ++--
 drivers/gpu/drm/i915/i915_drv.h            |   8 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |   2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 139 +--------------------------
 drivers/gpu/drm/i915/i915_irq.c            |   2 +-
 drivers/gpu/drm/i915/i915_params.c         |   4 -
 drivers/gpu/drm/i915/i915_params.h         |   1 -
 drivers/gpu/drm/i915/intel_guc_ads.c       | 149 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_ads.h       |  33 +++++++
 drivers/gpu/drm/i915/intel_guc_fw.c        |  10 +-
 drivers/gpu/drm/i915/intel_guc_fw.h        |   2 +-
 drivers/gpu/drm/i915/intel_guc_log.c       |   8 +-
 drivers/gpu/drm/i915/intel_huc.c           |   3 +-
 drivers/gpu/drm/i915/intel_uc.c            | 101 ++++++++++++-------
 drivers/gpu/drm/i915/intel_uncore.c        |   3 +-
 17 files changed, 279 insertions(+), 205 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h

-- 
1.9.1

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

[PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha Sundaresan]

Unifying the various seq_puts messages in debugfs to the simplest one for
feature support.

v2: Clarifying the commit message (Anusha)

v3: Re-factoring code as per review (Michal)

v4: Rebase

v5: Split from following patch

v6: Re-factoring code (Michal, Sagar)
    Clarifying commit message (Sagar)

v7: Generalizing subject to drm/i915 (Sagar)

v8: Omitting DRRS seq_puts unification (Michal)

Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c65e381..8edd029 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1641,7 +1641,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;
 	}
 
@@ -1809,7 +1809,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;
 	}
 
@@ -2361,8 +2361,10 @@ 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 drm_printer p;
 
-	if (!HAS_HUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)) {
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
 
 	p = drm_seq_file_printer(m);
 	intel_uc_fw_dump(&dev_priv->huc.fw, &p);
@@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct drm_printer p;
 	u32 tmp, i;
 
-	if (!HAS_GUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)) {
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
 
 	p = drm_seq_file_printer(m);
 	intel_uc_fw_dump(&dev_priv->guc.fw, &p);
@@ -2650,7 +2654,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;
 	}
 
-- 
1.9.1

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

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Michal Wajdeczko]

On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Unifying the various seq_puts messages in debugfs to the simplest one for
> feature support.
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Split from following patch
>
> v6: Re-factoring code (Michal, Sagar)
>     Clarifying commit message (Sagar)
>
> v7: Generalizing subject to drm/i915 (Sagar)
>
> v8: Omitting DRRS seq_puts unification (Michal)
>
> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c65e381..8edd029 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1641,7 +1641,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;
>  	}
> @@ -1809,7 +1809,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;
>  	}
> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
> -	if (!HAS_HUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)) {

Hmm, I think that in above code we should use HAS_HUC defined as:

/* HuC is inherent part of the GuC ... */
#define HAS_HUC(dev_priv)	HAS_GUC(dev_priv)

to make it clear that code checks HuC sub-feature (not other part
of the GuC or GuC itself). And additionally we can use above define
to explicitly document relation between GuC and HuC.

Michal

> +		seq_puts(m, "not supported\n");
>  		return 0;
> +	}
> 	p = drm_seq_file_printer(m);
>  	intel_uc_fw_dump(&dev_priv->huc.fw, &p);
> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct  
> seq_file *m, void *data)
>  	struct drm_printer p;
>  	u32 tmp, i;
> -	if (!HAS_GUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)) {
> +		seq_puts(m, "not supported\n");
>  		return 0;
> +	}
> 	p = drm_seq_file_printer(m);
>  	intel_uc_fw_dump(&dev_priv->guc.fw, &p);
> @@ -2650,7 +2654,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;
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Daniele Ceraolo Spurio]

On 25/10/17 06:31, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
> 
>> Unifying the various seq_puts messages in debugfs to the simplest one for
>> feature support.
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Split from following patch
>>
>> v6: Re-factoring code (Michal, Sagar)
>>     Clarifying commit message (Sagar)
>>
>> v7: Generalizing subject to drm/i915 (Sagar)
>>
>> v8: Omitting DRRS seq_puts unification (Michal)
>>
>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index c65e381..8edd029 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1641,7 +1641,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;
>>      }
>> @@ -1809,7 +1809,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;
>>      }
>> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
>> -    if (!HAS_HUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)) {
> 
> Hmm, I think that in above code we should use HAS_HUC defined as:
> 
> /* HuC is inherent part of the GuC ... */
> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
> 
> to make it clear that code checks HuC sub-feature (not other part
> of the GuC or GuC itself). And additionally we can use above define
> to explicitly document relation between GuC and HuC.
> 
> Michal
> 

The suggested comment feels confusing to me. HuC is a different 
microcontroller and not a part of GuC. The only tie the 2 have is that 
GuC needs to do the authentication. It is however true that any platform 
that has a GuC also has a HuC so the suggested define itself is fine.

Daniele

>> +        seq_puts(m, "not supported\n");
>>          return 0;
>> +    }
>>     p = drm_seq_file_printer(m);
>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>> seq_file *m, void *data)
>>      struct drm_printer p;
>>      u32 tmp, i;
>> -    if (!HAS_GUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)) {
>> +        seq_puts(m, "not supported\n");
>>          return 0;
>> +    }
>>     p = drm_seq_file_printer(m);
>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>> @@ -2650,7 +2654,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;
>>      }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sagar Arun Kamble]

On 10/26/2017 11:24 PM, Daniele Ceraolo Spurio wrote:
>
>
> On 25/10/17 06:31, Michal Wajdeczko wrote:
>> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
>> <sujaritha.sundaresan@intel.com> wrote:
>>
>>> Unifying the various seq_puts messages in debugfs to the simplest 
>>> one for
>>> feature support.
As Michal noted in the v7 review, if the goal is to unification of 
consistent output then I see some more in the debugfs that
might need to be updated: *_wm_latency_open, i915_ipc_status, 
i915_runtime_pm_status(returning early could be done later)
i915_llc (add change to return early).
Also, I think this patch can be separated from this series as it has 
very little dependency.
>>>
>>> v2: Clarifying the commit message (Anusha)
>>>
>>> v3: Re-factoring code as per review (Michal)
>>>
>>> v4: Rebase
>>>
>>> v5: Split from following patch
>>>
>>> v6: Re-factoring code (Michal, Sagar)
>>>     Clarifying commit message (Sagar)
>>>
>>> v7: Generalizing subject to drm/i915 (Sagar)
>>>
>>> v8: Omitting DRRS seq_puts unification (Michal)
>>>
>>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index c65e381..8edd029 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1641,7 +1641,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;
>>>      }
>>> @@ -1809,7 +1809,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;
>>>      }
>>> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
>>> -    if (!HAS_HUC_UCODE(dev_priv))
>>> +    if (!HAS_GUC(dev_priv)) {
>>
>> Hmm, I think that in above code we should use HAS_HUC defined as:
>>
>> /* HuC is inherent part of the GuC ... */
>> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
>>
>> to make it clear that code checks HuC sub-feature (not other part
>> of the GuC or GuC itself). And additionally we can use above define
>> to explicitly document relation between GuC and HuC.
>>
>> Michal
>>
>
> The suggested comment feels confusing to me. HuC is a different 
> microcontroller and not a part of GuC. The only tie the 2 have is that 
> GuC needs to do the authentication. It is however true that any 
> platform that has a GuC also has a HuC so the suggested define itself 
> is fine.
>
> Daniele
>
>>> +        seq_puts(m, "not supported\n");
>>>          return 0;
>>> +    }
>>>     p = drm_seq_file_printer(m);
>>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>>> seq_file *m, void *data)
>>>      struct drm_printer p;
>>>      u32 tmp, i;
>>> -    if (!HAS_GUC_UCODE(dev_priv))
>>> +    if (!HAS_GUC(dev_priv)) {
>>> +        seq_puts(m, "not supported\n");
>>>          return 0;
>>> +    }
>>>     p = drm_seq_file_printer(m);
>>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>>> @@ -2650,7 +2654,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;
>>>      }
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha]

On 10/29/2017 09:49 PM, Sagar Arun Kamble wrote:
>
>
> On 10/26/2017 11:24 PM, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 25/10/17 06:31, Michal Wajdeczko wrote:
>>> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
>>> <sujaritha.sundaresan@intel.com> wrote:
>>>
>>>> Unifying the various seq_puts messages in debugfs to the simplest 
>>>> one for
>>>> feature support.
> As Michal noted in the v7 review, if the goal is to unification of 
> consistent output then I see some more in the debugfs that
> might need to be updated: *_wm_latency_open, i915_ipc_status, 
> i915_runtime_pm_status(returning early could be done later)
> i915_llc (add change to return early).
> Also, I think this patch can be separated from this series as it has 
> very little dependency.

Sure, I will include the other updates to the patch. But I feel that it 
might be better to keep
the patch with series, since I'm including the change to HAS_GUC here.
>>>>
>>>> v2: Clarifying the commit message (Anusha)
>>>>
>>>> v3: Re-factoring code as per review (Michal)
>>>>
>>>> v4: Rebase
>>>>
>>>> v5: Split from following patch
>>>>
>>>> v6: Re-factoring code (Michal, Sagar)
>>>>     Clarifying commit message (Sagar)
>>>>
>>>> v7: Generalizing subject to drm/i915 (Sagar)
>>>>
>>>> v8: Omitting DRRS seq_puts unification (Michal)
>>>>
>>>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> index c65e381..8edd029 100644
>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> @@ -1641,7 +1641,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;
>>>>      }
>>>> @@ -1809,7 +1809,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;
>>>>      }
>>>> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
>>>> -    if (!HAS_HUC_UCODE(dev_priv))
>>>> +    if (!HAS_GUC(dev_priv)) {
>>>
>>> Hmm, I think that in above code we should use HAS_HUC defined as:
>>>
>>> /* HuC is inherent part of the GuC ... */
>>> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
>>>
>>> to make it clear that code checks HuC sub-feature (not other part
>>> of the GuC or GuC itself). And additionally we can use above define
>>> to explicitly document relation between GuC and HuC.
>>>
>>> Michal
>>>
>>
>> The suggested comment feels confusing to me. HuC is a different 
>> microcontroller and not a part of GuC. The only tie the 2 have is 
>> that GuC needs to do the authentication. It is however true that any 
>> platform that has a GuC also has a HuC so the suggested define itself 
>> is fine.
>>
>> Daniele
>>
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>> +    }
>>>>     p = drm_seq_file_printer(m);
>>>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>>>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>>>> seq_file *m, void *data)
>>>>      struct drm_printer p;
>>>>      u32 tmp, i;
>>>> -    if (!HAS_GUC_UCODE(dev_priv))
>>>> +    if (!HAS_GUC(dev_priv)) {
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>> +    }
>>>>     p = drm_seq_file_printer(m);
>>>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>>>> @@ -2650,7 +2654,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;
>>>>      }
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Thanks for the review,

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

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha]

On 10/26/2017 10:54 AM, Daniele Ceraolo Spurio wrote:
>
>
> On 25/10/17 06:31, Michal Wajdeczko wrote:
>> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
>> <sujaritha.sundaresan@intel.com> wrote:
>>
>>> Unifying the various seq_puts messages in debugfs to the simplest 
>>> one for
>>> feature support.
>>>
>>> v2: Clarifying the commit message (Anusha)
>>>
>>> v3: Re-factoring code as per review (Michal)
>>>
>>> v4: Rebase
>>>
>>> v5: Split from following patch
>>>
>>> v6: Re-factoring code (Michal, Sagar)
>>>     Clarifying commit message (Sagar)
>>>
>>> v7: Generalizing subject to drm/i915 (Sagar)
>>>
>>> v8: Omitting DRRS seq_puts unification (Michal)
>>>
>>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index c65e381..8edd029 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1641,7 +1641,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;
>>>      }
>>> @@ -1809,7 +1809,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;
>>>      }
>>> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
>>> -    if (!HAS_HUC_UCODE(dev_priv))
>>> +    if (!HAS_GUC(dev_priv)) {
>>
>> Hmm, I think that in above code we should use HAS_HUC defined as:
>>
>> /* HuC is inherent part of the GuC ... */
>> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
>>
>> to make it clear that code checks HuC sub-feature (not other part
>> of the GuC or GuC itself). And additionally we can use above define
>> to explicitly document relation between GuC and HuC.
>>
>> Michal
>>
>
> The suggested comment feels confusing to me. HuC is a different 
> microcontroller and not a part of GuC. The only tie the 2 have is that 
> GuC needs to do the authentication. It is however true that any 
> platform that has a GuC also has a HuC so the suggested define itself 
> is fine.
>
> Daniele
>

I agree. I will include the condition and a comment that clearly 
mentions the GuC and HuC tie.
>>> +        seq_puts(m, "not supported\n");
>>>          return 0;
>>> +    }
>>>     p = drm_seq_file_printer(m);
>>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>>> seq_file *m, void *data)
>>>      struct drm_printer p;
>>>      u32 tmp, i;
>>> -    if (!HAS_GUC_UCODE(dev_priv))
>>> +    if (!HAS_GUC(dev_priv)) {
>>> +        seq_puts(m, "not supported\n");
>>>          return 0;
>>> +    }
>>>     p = drm_seq_file_printer(m);
>>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>>> @@ -2650,7 +2654,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;
>>>      }
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Thanks for the review.

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

Re: [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha]

On 10/25/2017 06:31 AM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> Unifying the various seq_puts messages in debugfs to the simplest one 
>> for
>> feature support.
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Split from following patch
>>
>> v6: Re-factoring code (Michal, Sagar)
>>     Clarifying commit message (Sagar)
>>
>> v7: Generalizing subject to drm/i915 (Sagar)
>>
>> v8: Omitting DRRS seq_puts unification (Michal)
>>
>> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index c65e381..8edd029 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1641,7 +1641,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;
>>      }
>> @@ -1809,7 +1809,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;
>>      }
>> @@ -2361,8 +2361,10 @@ 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 drm_printer p;
>> -    if (!HAS_HUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)) {
>
> Hmm, I think that in above code we should use HAS_HUC defined as:
>
> /* HuC is inherent part of the GuC ... */
> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
>
> to make it clear that code checks HuC sub-feature (not other part
> of the GuC or GuC itself). And additionally we can use above define
> to explicitly document relation between GuC and HuC.
>
> Michal

Sure, I will include the HAS_HUC condition and a comment that clarifies 
the HuC and GuC tie in the next revision.
>
>> +        seq_puts(m, "not supported\n");
>>          return 0;
>> +    }
>>     p = drm_seq_file_printer(m);
>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>> seq_file *m, void *data)
>>      struct drm_printer p;
>>      u32 tmp, i;
>> -    if (!HAS_GUC_UCODE(dev_priv))
>> +    if (!HAS_GUC(dev_priv)) {
>> +        seq_puts(m, "not supported\n");
>>          return 0;
>> +    }
>>     p = drm_seq_file_printer(m);
>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>> @@ -2650,7 +2654,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;
>>      }

Thanks for the review.

Regards,

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

[PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha Sundaresan]

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

Also if we have HuC have firmware to be loaded, we need to
have GuC to actually load it. So if the user wants to avoid
the GuC from getting loaded, they must not have a HuC
firmware to be loaded, in addition to not using submission.

v2: Clarifying the commit message (Anusha)

v3: Unify seq_puts messages, Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating message unification into a separate patch

v6: Re-factoring code (Sagar, Michal)
    Rebase

v7: Applying review comments (Sagar)
    Rebase

v8: Change to NEEDS_GUC_FW (Chris)
    Applying review comments (Michal)
    Clarifying commit message (Joonas)

Suggested by: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
 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      |  4 ---
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_uc.c         | 57 +++++++++++++++------------------
 8 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8edd029..25c47a0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2465,7 +2465,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;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f01c800..ede5004 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3205,9 +3205,11 @@ static inline unsigned int i915_sg_segment_size(void)
  */
 #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_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
+#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL)
+
+#define NEEDS_GUC_FW(dev_priv) \
+		(HAS_GUC(dev_priv) && \
+		(i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5bf96a2..4f0692e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -314,7 +314,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
 	 * 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)
+	if (NEEDS_GUC_FW(dev_priv))
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 527a2d2..9d78233 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
 	 * currently don't have any bits spare to pass in this upper
 	 * restriction!
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
+	if (NEEDS_GUC_FW(dev_priv)) {
 		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
 		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1296a5..ec76aac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		dev_priv->l3_parity.remap_info[i] = NULL;
 
-	if (HAS_GUC_SCHED(dev_priv))
+	if (HAS_GUC(dev_priv))
 		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b4faeb6..1c25f45 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = {
 	"(0=use value from vbt [default], 1=low power swing(200mV),"
 	"2=default swing(400mV))");
 
-i915_param_named_unsafe(enable_guc_loading, int, 0400,
-	"Enable GuC firmware loading "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
 i915_param_named_unsafe(enable_guc_submission, int, 0400,
 	"Enable GuC submission "
 	"(-1=auto, 0=never [default], 1=if available, 2=required)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..9e1e231 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,6 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
 	param(int, enable_guc_submission, 0) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 25bd162..9369ade 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -49,36 +49,35 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
+	/* Verify Hardware support */
 	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
-			DRM_INFO("Ignoring GuC options, no hardware\n");
-
-		i915_modparams.enable_guc_loading = 0;
-		i915_modparams.enable_guc_submission = 0;
+		if (i915_modparams.enable_guc_submission > 0)
+			DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");
+			i915_modparams.enable_guc_submission = 0;
 		return;
 	}
 
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_loading < 0)
-		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-
 	/* Verify firmware version */
-	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
+	if (HAS_GUC_UCODE(dev_priv)) {
+		if (i915_modparams.enable_guc_submission > 0) {
+			DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");
+			i915_modparams.enable_guc_submission = 0;
+		return;
+		}
 
-		if (intel_guc_fw_select(&dev_priv->guc))
-			i915_modparams.enable_guc_loading = 0;
+		if (i915_modparams.enable_guc_submission < 0) {
+			i915_modparams.enable_guc_submission = 0;
+			return;
+		}
 	}
 
-	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
-		i915_modparams.enable_guc_submission = 0;
+	/*
+	 * A negative value means "use platform default" (enabled if we have
+	 * survived to get here)
+	 */
 
-	/* A negative value means "use platform default" */
 	if (i915_modparams.enable_guc_submission < 0)
-		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -154,7 +153,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	int ret, attempts;
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_FW(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -250,22 +249,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_loading > 1 ||
-	    i915_modparams.enable_guc_submission > 1) {
+	if (i915_modparams.enable_guc_submission > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
 		ret = -EIO;
+	} else if (i915_modparams.enable_guc_submission == 1) {
+		DRM_NOTE("Falling back from GuC submission to execlist mode.\n");
+		ret = 0;
 	} else {
-		DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
 		ret = 0;
 	}
 
-	if (i915_modparams.enable_guc_submission) {
-		i915_modparams.enable_guc_submission = 0;
-		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-	}
-
-	i915_modparams.enable_guc_loading = 0;
-
 	return ret;
 }
 
@@ -273,7 +266,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
 	if (i915_modparams.enable_guc_submission)
-- 
1.9.1

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

Re: [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Michal Wajdeczko]

On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need submission=1, we also need loading=1.We also need
> loading=1 when we want to want to verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
>
> Also if we have HuC have firmware to be loaded, we need to
> have GuC to actually load it. So if the user wants to avoid
> the GuC from getting loaded, they must not have a HuC
> firmware to be loaded, in addition to not using submission.

Hmm, I'm not sure that removal of HuC firmware file is the best
way for the user to stop undesired GuC loading.

I know that we want to minimize number of modparams, but maybe
new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
will solve here ...

Alternatively we can replace both existing modparams with single:

i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)

then we could cover almost all cases:

0 = GuC loading disabled (no GuC submission, no HuC)
1 = GuC loading auto
2 = GuC loading enabled, GuC submission required, HuC disabled
3 = GuC loading enabled, GuC submission enabled,  HuC disabled
4 = GuC loading enabled, GuC submission disabled, HuC required
5 = GuC loading enabled, GuC submission disabled, HuC enabled
6 = GuC loading enabled, GuC submission required, HuC required
7 = GuC loading enabled, GuC submission enabled,  HuC enabled


>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> v6: Re-factoring code (Sagar, Michal)
>     Rebase
>
> v7: Applying review comments (Sagar)
>     Rebase
>
> v8: Change to NEEDS_GUC_FW (Chris)
>     Applying review comments (Michal)
>     Clarifying commit message (Joonas)
>
> Suggested by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
>  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      |  4 ---
>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>  drivers/gpu/drm/i915/intel_uc.c         | 57  
> +++++++++++++++------------------
>  8 files changed, 34 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8edd029..25c47a0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2465,7 +2465,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");

As I already said before, there is also 3rd possible status "failed"

	!HAS_GUC(dev_priv) ? "not supported" :
		!HAS_GUC_SCHED(dev_priv) ? "disabled" :
			"failed"

where HAS_GUC_SCHED is

#define HAS_GUC_SCHED(dev_priv) \
		(HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)

or something similar

>  		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index f01c800..ede5004 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3205,9 +3205,11 @@ static inline unsigned int  
> i915_sg_segment_size(void)
>   */
>  #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_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL)
> +
> +#define NEEDS_GUC_FW(dev_priv) \

Hmm, based on its usage, this name is now little confusing.
Maybe USES_GUC ? See USES_PPGTT|USES_FULL_PPGTT|USES_FULL_48BIT_PPGTT

> +		(HAS_GUC(dev_priv) && \
> +		(i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))

While unlikely, above will be true even with guc.fw.path == NULL

Also, based on your statement "all platforms with HuC have a GuC
and viceversa" I would assume that corresponding firmwares will
be delivered also in pairs (except short enabling periods).

Thus on every platform with has_guc=1 there will be guc.fw.path != NULL
and huc.fw.path != NULL, effectively making this macro almost the
same as HAS_GUC (as other cases are just error cases).

If we want to make GuC loading conditional, we should use better
condition ;)


> #define HAS_RESOURCE_STREAMER(dev_priv)  
> ((dev_priv)->info.has_resource_streamer)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c  
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 5bf96a2..4f0692e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct  
> drm_i915_private *i915,
>  	 * 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)
> +	if (NEEDS_GUC_FW(dev_priv))
>  		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>  	else
>  		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 527a2d2..9d78233 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private  
> *dev_priv)
>  	 * currently don't have any bits spare to pass in this upper
>  	 * restriction!
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_FW(dev_priv)) {
>  		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>  		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a5..ec76aac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private  
> *dev_priv)
>  	for (i = 0; i < MAX_L3_SLICES; ++i)
>  		dev_priv->l3_parity.remap_info[i] = NULL;
> -	if (HAS_GUC_SCHED(dev_priv))
> +	if (HAS_GUC(dev_priv))
>  		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> 	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index b4faeb6..1c25f45 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(0=use value from vbt [default], 1=low power swing(200mV),"
>  	"2=default swing(400mV))");
> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
> -	"Enable GuC firmware loading "
> -	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> -
>  i915_param_named_unsafe(enable_guc_submission, int, 0400,
>  	"Enable GuC submission "
>  	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h  
> b/drivers/gpu/drm/i915/i915_params.h
> index c729226..9e1e231 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@
>  	param(int, disable_power_well, -1) \
>  	param(int, enable_ips, 1) \
>  	param(int, invert_brightness, 0) \
> -	param(int, enable_guc_loading, 0) \
>  	param(int, enable_guc_submission, 0) \
>  	param(int, guc_log_level, -1) \
>  	param(char *, guc_firmware_path, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 25bd162..9369ade 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -49,36 +49,35 @@ static int __intel_uc_reset_hw(struct  
> drm_i915_private *dev_priv)
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
> +	/* Verify Hardware support */
>  	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> -			DRM_INFO("Ignoring GuC options, no hardware\n");
> -
> -		i915_modparams.enable_guc_loading = 0;
> -		i915_modparams.enable_guc_submission = 0;
> +		if (i915_modparams.enable_guc_submission > 0)
> +			DRM_INFO("Ignoring option %s - no hardware",  
> "enable_guc_submission");
> +			i915_modparams.enable_guc_submission = 0;
>  		return;
>  	}
> -	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_loading < 0)
> -		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -
>  	/* Verify firmware version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> +	if (HAS_GUC_UCODE(dev_priv)) {

Hmm, if !HAS_UCODE ?

> +		if (i915_modparams.enable_guc_submission > 0) {
> +			DRM_INFO("Ignoring option %s - no firmware",  
> "enable_guc_submission");
> +			i915_modparams.enable_guc_submission = 0;
> +		return;
> +		}
> -		if (intel_guc_fw_select(&dev_priv->guc))

Hmm, I can't see now when fw will be selected...
Note that you are using here HAS_GUC_UCODE that depends on fw path

> -			i915_modparams.enable_guc_loading = 0;
> +		if (i915_modparams.enable_guc_submission < 0) {
> +			i915_modparams.enable_guc_submission = 0;
> +			return;
> +		}
>  	}
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> -		i915_modparams.enable_guc_submission = 0;
> +	/*
> +	 * A negative value means "use platform default" (enabled if we have
> +	 * survived to get here)
> +	 */
> -	/* A negative value means "use platform default" */
>  	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);

At this point we are sure that we have GuC (with fw) so we can use

i915_modparams.enable_guc_submission = 1;

>  }
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -154,7 +153,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
>  	int ret, attempts;
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_FW(dev_priv))
>  		return 0;
> 	guc_disable_communication(guc);
> @@ -250,22 +249,16 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  err_guc:
>  	i915_ggtt_disable_guc(dev_priv);
> -	if (i915_modparams.enable_guc_loading > 1 ||
> -	    i915_modparams.enable_guc_submission > 1) {
> +	if (i915_modparams.enable_guc_submission > 1) {
>  		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>  		ret = -EIO;
> +	} else if (i915_modparams.enable_guc_submission == 1) {
> +		DRM_NOTE("Falling back from GuC submission to execlist mode.\n");
> +		ret = 0;
>  	} else {
> -		DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
>  		ret = 0;
>  	}
> -	if (i915_modparams.enable_guc_submission) {
> -		i915_modparams.enable_guc_submission = 0;
> -		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -	}
> -
> -	i915_modparams.enable_guc_loading = 0;
> -
>  	return ret;
>  }
> @@ -273,7 +266,7 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  {
>  	guc_free_load_err_log(&dev_priv->guc);
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_FW(dev_priv))
>  		return;
> 	if (i915_modparams.enable_guc_submission)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/25/2017 08:26 AM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need submission=1, we also need loading=1.We also need
>> loading=1 when we want to want to verify the HuC, which
>> is every time we have a HuC (but all platforms with HuC
>> have a GuC and viceversa).
>>
>> Also if we have HuC have firmware to be loaded, we need to
>> have GuC to actually load it. So if the user wants to avoid
>> the GuC from getting loaded, they must not have a HuC
>> firmware to be loaded, in addition to not using submission.
>
> Hmm, I'm not sure that removal of HuC firmware file is the best
> way for the user to stop undesired GuC loading.
>
> I know that we want to minimize number of modparams, but maybe
> new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
> will solve here ...
>
> Alternatively we can replace both existing modparams with single:
>
> i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)
>
> then we could cover almost all cases:
>
> 0 = GuC loading disabled (no GuC submission, no HuC)
> 1 = GuC loading auto
> 2 = GuC loading enabled, GuC submission required, HuC disabled
> 3 = GuC loading enabled, GuC submission enabled,  HuC disabled
> 4 = GuC loading enabled, GuC submission disabled, HuC required
> 5 = GuC loading enabled, GuC submission disabled, HuC enabled
> 6 = GuC loading enabled, GuC submission required, HuC required
> 7 = GuC loading enabled, GuC submission enabled,  HuC enabled

This is a really good idea. I will include the new modparams in the next 
revision.
>
>
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating message unification into a separate patch
>>
>> v6: Re-factoring code (Sagar, Michal)
>>     Rebase
>>
>> v7: Applying review comments (Sagar)
>>     Rebase
>>
>> v8: Change to NEEDS_GUC_FW (Chris)
>>     Applying review comments (Michal)
>>     Clarifying commit message (Joonas)
>>
>> Suggested by: Oscar Mateo <oscar.mateo@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
>>  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      |  4 ---
>>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>>  drivers/gpu/drm/i915/intel_uc.c         | 57 
>> +++++++++++++++------------------
>>  8 files changed, 34 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 8edd029..25c47a0 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2465,7 +2465,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");
>
> As I already said before, there is also 3rd possible status "failed"
>
>     !HAS_GUC(dev_priv) ? "not supported" :
>         !HAS_GUC_SCHED(dev_priv) ? "disabled" :
>             "failed"
>
> where HAS_GUC_SCHED is
>
> #define HAS_GUC_SCHED(dev_priv) \
>         (HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
>
> or something similar
>

Sorry, I missed the third case. I will include it and the change to the 
macro condition
in the next revision.
>>          return false;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index f01c800..ede5004 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3205,9 +3205,11 @@ static inline unsigned int 
>> i915_sg_segment_size(void)
>>   */
>>  #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_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
>> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL)
>> +
>> +#define NEEDS_GUC_FW(dev_priv) \
>
> Hmm, based on its usage, this name is now little confusing.
> Maybe USES_GUC ? See USES_PPGTT|USES_FULL_PPGTT|USES_FULL_48BIT_PPGTT
>
I would really prefer to keep NEEDS_GUC_FW

>> +        (HAS_GUC(dev_priv) && \
>> +        (i915_modparams.enable_guc_submission || 
>> HAS_HUC_UCODE(dev_priv)))
>
> While unlikely, above will be true even with guc.fw.path == NULL
>
> Also, based on your statement "all platforms with HuC have a GuC
> and viceversa" I would assume that corresponding firmwares will
> be delivered also in pairs (except short enabling periods).
>
> Thus on every platform with has_guc=1 there will be guc.fw.path != NULL
> and huc.fw.path != NULL, effectively making this macro almost the
> same as HAS_GUC (as other cases are just error cases).
>
> If we want to make GuC loading conditional, we should use better
> condition ;)
>

Yes, I will improve the condition.
>
>> #define HAS_RESOURCE_STREAMER(dev_priv) 
>> ((dev_priv)->info.has_resource_streamer)
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 5bf96a2..4f0692e 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct 
>> drm_i915_private *i915,
>>       * 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)
>> +    if (NEEDS_GUC_FW(dev_priv))
>>          ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>>      else
>>          ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 527a2d2..9d78233 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private 
>> *dev_priv)
>>       * currently don't have any bits spare to pass in this upper
>>       * restriction!
>>       */
>> -    if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
>> +    if (NEEDS_GUC_FW(dev_priv)) {
>>          ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>>          ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>>      }
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index b1296a5..ec76aac 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private 
>> *dev_priv)
>>      for (i = 0; i < MAX_L3_SLICES; ++i)
>>          dev_priv->l3_parity.remap_info[i] = NULL;
>> -    if (HAS_GUC_SCHED(dev_priv))
>> +    if (HAS_GUC(dev_priv))
>>          dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>>     /* Let's track the enabled rps events */
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b4faeb6..1c25f45 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = {
>>      "(0=use value from vbt [default], 1=low power swing(200mV),"
>>      "2=default swing(400mV))");
>> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
>> -    "Enable GuC firmware loading "
>> -    "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> -
>>  i915_param_named_unsafe(enable_guc_submission, int, 0400,
>>      "Enable GuC submission "
>>      "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index c729226..9e1e231 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,6 @@
>>      param(int, disable_power_well, -1) \
>>      param(int, enable_ips, 1) \
>>      param(int, invert_brightness, 0) \
>> -    param(int, enable_guc_loading, 0) \
>>      param(int, enable_guc_submission, 0) \
>>      param(int, guc_log_level, -1) \
>>      param(char *, guc_firmware_path, NULL) \
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 25bd162..9369ade 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -49,36 +49,35 @@ static int __intel_uc_reset_hw(struct 
>> drm_i915_private *dev_priv)
>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>>  {
>> +    /* Verify Hardware support */
>>      if (!HAS_GUC(dev_priv)) {
>> -        if (i915_modparams.enable_guc_loading > 0 ||
>> -            i915_modparams.enable_guc_submission > 0)
>> -            DRM_INFO("Ignoring GuC options, no hardware\n");
>> -
>> -        i915_modparams.enable_guc_loading = 0;
>> -        i915_modparams.enable_guc_submission = 0;
>> +        if (i915_modparams.enable_guc_submission > 0)
>> +            DRM_INFO("Ignoring option %s - no hardware", 
>> "enable_guc_submission");
>> +            i915_modparams.enable_guc_submission = 0;
>>          return;
>>      }
>> -    /* A negative value means "use platform default" */
>> -    if (i915_modparams.enable_guc_loading < 0)
>> -        i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>> -
>>      /* Verify firmware version */
>> -    if (i915_modparams.enable_guc_loading) {
>> -        if (HAS_HUC_UCODE(dev_priv))
>> -            intel_huc_select_fw(&dev_priv->huc);
>> +    if (HAS_GUC_UCODE(dev_priv)) {
>
> Hmm, if !HAS_UCODE ?

Will do.
>
>> +        if (i915_modparams.enable_guc_submission > 0) {
>> +            DRM_INFO("Ignoring option %s - no firmware", 
>> "enable_guc_submission");
>> +            i915_modparams.enable_guc_submission = 0;
>> +        return;
>> +        }
>> -        if (intel_guc_fw_select(&dev_priv->guc))
>
> Hmm, I can't see now when fw will be selected...
> Note that you are using here HAS_GUC_UCODE that depends on fw path
>

I will see where I can make it evident that fw will be selected.
>> - i915_modparams.enable_guc_loading = 0;
>> +        if (i915_modparams.enable_guc_submission < 0) {
>> +            i915_modparams.enable_guc_submission = 0;
>> +            return;
>> +        }
>>      }
>> -    /* Can't enable guc submission without guc loaded */
>> -    if (!i915_modparams.enable_guc_loading)
>> -        i915_modparams.enable_guc_submission = 0;
>> +    /*
>> +     * A negative value means "use platform default" (enabled if we 
>> have
>> +     * survived to get here)
>> +     */
>> -    /* A negative value means "use platform default" */
>>      if (i915_modparams.enable_guc_submission < 0)
>> -        i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> +        i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
>
> At this point we are sure that we have GuC (with fw) so we can use
>
> i915_modparams.enable_guc_submission = 1;

Will do.
>
>>  }
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>> @@ -154,7 +153,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      struct intel_guc *guc = &dev_priv->guc;
>>      int ret, attempts;
>> -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_FW(dev_priv))
>>          return 0;
>>     guc_disable_communication(guc);
>> @@ -250,22 +249,16 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>  err_guc:
>>      i915_ggtt_disable_guc(dev_priv);
>> -    if (i915_modparams.enable_guc_loading > 1 ||
>> -        i915_modparams.enable_guc_submission > 1) {
>> +    if (i915_modparams.enable_guc_submission > 1) {
>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>>          ret = -EIO;
>> +    } else if (i915_modparams.enable_guc_submission == 1) {
>> +        DRM_NOTE("Falling back from GuC submission to execlist 
>> mode.\n");
>> +        ret = 0;
>>      } else {
>> -        DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
>>          ret = 0;
>>      }
>> -    if (i915_modparams.enable_guc_submission) {
>> -        i915_modparams.enable_guc_submission = 0;
>> -        DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>> -    }
>> -
>> -    i915_modparams.enable_guc_loading = 0;
>> -
>>      return ret;
>>  }
>> @@ -273,7 +266,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>  {
>>      guc_free_load_err_log(&dev_priv->guc);
>> -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_FW(dev_priv))
>>          return;
>>     if (i915_modparams.enable_guc_submission)

Thanks for the review.

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

Re: [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Joonas Lahtinen]

On Thu, 2017-11-02 at 09:34 -0700, Sujaritha wrote:
> 
> On 10/25/2017 08:26 AM, Michal Wajdeczko wrote:
> > On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan 
> > <sujaritha.sundaresan@intel.com> wrote:
> > 
> > > We currently have two module parameters that control GuC:
> > > "enable_guc_loading" and "enable_guc_submission". Whenever
> > > we need submission=1, we also need loading=1.We also need
> > > loading=1 when we want to want to verify the HuC, which
> > > is every time we have a HuC (but all platforms with HuC
> > > have a GuC and viceversa).
> > > 
> > > Also if we have HuC have firmware to be loaded, we need to
> > > have GuC to actually load it. So if the user wants to avoid
> > > the GuC from getting loaded, they must not have a HuC
> > > firmware to be loaded, in addition to not using submission.
> > 
> > Hmm, I'm not sure that removal of HuC firmware file is the best
> > way for the user to stop undesired GuC loading.
> > 
> > I know that we want to minimize number of modparams, but maybe
> > new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
> > will solve here ...
> > 
> > Alternatively we can replace both existing modparams with single:
> > 
> > i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)
> > 
> > then we could cover almost all cases:
> > 
> > 0 = GuC loading disabled (no GuC submission, no HuC)
> > 1 = GuC loading auto
> > 2 = GuC loading enabled, GuC submission required, HuC disabled
> > 3 = GuC loading enabled, GuC submission enabled,  HuC disabled
> > 4 = GuC loading enabled, GuC submission disabled, HuC required
> > 5 = GuC loading enabled, GuC submission disabled, HuC enabled
> > 6 = GuC loading enabled, GuC submission required, HuC required
> > 7 = GuC loading enabled, GuC submission enabled,  HuC enabled

Do we really need all these combinations.

My understanding is that we got three cases:

1. Load and use GuC, HuC goes on the side
2. Load GuC, just to get HuC
3. Don't load GuC at all

Which could be mapped to .enable_guc:

-1 = default (driver does as sees fit)
0 = no GuC, no nothing
1 = load and use GuC, HuC comes on the side
2 = Load GuC only, for HuC

Or if you want just the GuC without HuC at times, then

0x1 = Use GuC
0x2 = Use Huc

Loading is then implied. Somebody could remind me why we should
consider required, disabled vs. enabled options?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Jani Nikula]

On Fri, 03 Nov 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> On Thu, 2017-11-02 at 09:34 -0700, Sujaritha wrote:
>> 
>> On 10/25/2017 08:26 AM, Michal Wajdeczko wrote:
>> > On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan 
>> > <sujaritha.sundaresan@intel.com> wrote:
>> > 
>> > > We currently have two module parameters that control GuC:
>> > > "enable_guc_loading" and "enable_guc_submission". Whenever
>> > > we need submission=1, we also need loading=1.We also need
>> > > loading=1 when we want to want to verify the HuC, which
>> > > is every time we have a HuC (but all platforms with HuC
>> > > have a GuC and viceversa).
>> > > 
>> > > Also if we have HuC have firmware to be loaded, we need to
>> > > have GuC to actually load it. So if the user wants to avoid
>> > > the GuC from getting loaded, they must not have a HuC
>> > > firmware to be loaded, in addition to not using submission.
>> > 
>> > Hmm, I'm not sure that removal of HuC firmware file is the best
>> > way for the user to stop undesired GuC loading.
>> > 
>> > I know that we want to minimize number of modparams, but maybe
>> > new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
>> > will solve here ...
>> > 
>> > Alternatively we can replace both existing modparams with single:
>> > 
>> > i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)
>> > 
>> > then we could cover almost all cases:
>> > 
>> > 0 = GuC loading disabled (no GuC submission, no HuC)
>> > 1 = GuC loading auto
>> > 2 = GuC loading enabled, GuC submission required, HuC disabled
>> > 3 = GuC loading enabled, GuC submission enabled,  HuC disabled
>> > 4 = GuC loading enabled, GuC submission disabled, HuC required
>> > 5 = GuC loading enabled, GuC submission disabled, HuC enabled
>> > 6 = GuC loading enabled, GuC submission required, HuC required
>> > 7 = GuC loading enabled, GuC submission enabled,  HuC enabled
>
> Do we really need all these combinations.

Ugh, I hope not. Pick the combinations you're committed to testing. If
it's not tested, it doesn't exist.

Side note, you also have guc_firmware_path and huc_firmware_path
options.

BR,
Jani.

> My understanding is that we got three cases:
>
> 1. Load and use GuC, HuC goes on the side
> 2. Load GuC, just to get HuC
> 3. Don't load GuC at all
>
> Which could be mapped to .enable_guc:
>
> -1 = default (driver does as sees fit)
> 0 = no GuC, no nothing
> 1 = load and use GuC, HuC comes on the side
> 2 = Load GuC only, for HuC
>
> Or if you want just the GuC without HuC at times, then
>
> 0x1 = Use GuC
> 0x2 = Use Huc
>
> Loading is then implied. Somebody could remind me why we should
> consider required, disabled vs. enabled options?
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Joonas Lahtinen]

On Fri, 2017-11-03 at 12:33 +0200, Jani Nikula wrote:
> On Fri, 03 Nov 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> > On Thu, 2017-11-02 at 09:34 -0700, Sujaritha wrote:
> > > 
> > > On 10/25/2017 08:26 AM, Michal Wajdeczko wrote:
> > > > On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan 
> > > > <sujaritha.sundaresan@intel.com> wrote:
> > > > 
> > > > > We currently have two module parameters that control GuC:
> > > > > "enable_guc_loading" and "enable_guc_submission". Whenever
> > > > > we need submission=1, we also need loading=1.We also need
> > > > > loading=1 when we want to want to verify the HuC, which
> > > > > is every time we have a HuC (but all platforms with HuC
> > > > > have a GuC and viceversa).
> > > > > 
> > > > > Also if we have HuC have firmware to be loaded, we need to
> > > > > have GuC to actually load it. So if the user wants to avoid
> > > > > the GuC from getting loaded, they must not have a HuC
> > > > > firmware to be loaded, in addition to not using submission.
> > > > 
> > > > Hmm, I'm not sure that removal of HuC firmware file is the best
> > > > way for the user to stop undesired GuC loading.
> > > > 
> > > > I know that we want to minimize number of modparams, but maybe
> > > > new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
> > > > will solve here ...
> > > > 
> > > > Alternatively we can replace both existing modparams with single:
> > > > 
> > > > i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)
> > > > 
> > > > then we could cover almost all cases:
> > > > 
> > > > 0 = GuC loading disabled (no GuC submission, no HuC)
> > > > 1 = GuC loading auto
> > > > 2 = GuC loading enabled, GuC submission required, HuC disabled
> > > > 3 = GuC loading enabled, GuC submission enabled,  HuC disabled
> > > > 4 = GuC loading enabled, GuC submission disabled, HuC required
> > > > 5 = GuC loading enabled, GuC submission disabled, HuC enabled
> > > > 6 = GuC loading enabled, GuC submission required, HuC required
> > > > 7 = GuC loading enabled, GuC submission enabled,  HuC enabled
> > 
> > Do we really need all these combinations.
> 
> Ugh, I hope not. Pick the combinations you're committed to testing. If
> it's not tested, it doesn't exist.
> 
> Side note, you also have guc_firmware_path and huc_firmware_path
> options.

Yep, I think I suggested them originally.

Then you only would have .enable_guc boolean for whether you want to
use GuC submission.

So I'm kinda looking forward to seeing a definitive list of what we
actually require by use-case and what we're committed to testing.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 3/6] drm/i915/guc : GEM_BUG_ON for GuC reset function

[Sujaritha Sundaresan]

Including GEM_BUG_ON for GuC reset function in intel_uncore.

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 20e3c65c..c631b0e 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (!HAS_GUC(dev_priv))
-		return -EINVAL;
+	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
-- 
1.9.1

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

[PATCH v8 4/6] drm/i915/guc : Updating GuC and HuC firmware select function

[Sujaritha Sundaresan]

Updating GuC and HuC firmware select function to support removing
i915_modparams.enable_guc_loading module parameter.

v2: Clarifying the commit message (Anusha)

v3: Unify seq_puts messages, Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating message unification into a separate patch

v6: Re-factoring code (Sagar, Michal)
    Rebase

v7: Separating from previuos patch (Sagar)
    Rebase

v8: Including change to intel_uc.c
    Applying review comments (Michal)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 10 +++-------
 drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
 drivers/gpu/drm/i915/intel_huc.c    |  3 ++-
 drivers/gpu/drm/i915/intel_uc.c     |  6 ++++++
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index ef67a36..b9f834f 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -60,10 +60,8 @@
  * intel_guc_fw_select() - selects GuC firmware for uploading
  *
  * @guc:	intel_guc struct
- *
- * Return: zero when we know firmware, non-zero in other case
  */
-int intel_guc_fw_select(struct intel_guc *guc)
+void intel_guc_fw_select(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
@@ -90,11 +88,9 @@ int intel_guc_fw_select(struct intel_guc *guc)
 		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
 		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
 	} else {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
+		DRM_ERROR("No GuC FW known for platform with GuC!\n");
+		return;
 	}
-
-	return 0;
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
index 023f5ba..7f6ccaf 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.h
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -27,7 +27,7 @@
 
 struct intel_guc;
 
-int intel_guc_fw_select(struct intel_guc *guc);
+void intel_guc_fw_select(struct intel_guc *guc);
 int intel_guc_fw_upload(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index c8a48cb..4e700ab 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -108,7 +108,8 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
 		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
 	} else {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No HuC FW known for platform with HuC!\n");
 		return;
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9369ade..dc978a0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -82,11 +82,17 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
+	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_huc *huc = &dev_priv->huc;
 	intel_guc_init_early(&dev_priv->guc);
+	intel_guc_fw_select(guc);
+	intel_huc_select_fw(huc);
 }
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_GUC(dev_priv))
+		return;
 	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
 	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
 }
-- 
1.9.1

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

Re: [PATCH v8 4/6] drm/i915/guc : Updating GuC and HuC firmware select function

[Michal Wajdeczko]

On Tue, 24 Oct 2017 19:21:23 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Updating GuC and HuC firmware select function to support removing
> i915_modparams.enable_guc_loading module parameter.

Hmm, is it correct order of patches ? this modparam was already removed in  
2/6

>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> v6: Re-factoring code (Sagar, Michal)
>     Rebase
>
> v7: Separating from previuos patch (Sagar)
>     Rebase
>
> v8: Including change to intel_uc.c
>     Applying review comments (Michal)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c | 10 +++-------
>  drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
>  drivers/gpu/drm/i915/intel_huc.c    |  3 ++-
>  drivers/gpu/drm/i915/intel_uc.c     |  6 ++++++
>  4 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c  
> b/drivers/gpu/drm/i915/intel_guc_fw.c
> index ef67a36..b9f834f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -60,10 +60,8 @@
>   * intel_guc_fw_select() - selects GuC firmware for uploading
>   *
>   * @guc:	intel_guc struct
> - *
> - * Return: zero when we know firmware, non-zero in other case
>   */
> -int intel_guc_fw_select(struct intel_guc *guc)
> +void intel_guc_fw_select(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -90,11 +88,9 @@ int intel_guc_fw_select(struct intel_guc *guc)
>  		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>  		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>  	} else {
> -		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> -		return -ENOENT;
> +		DRM_ERROR("No GuC FW known for platform with GuC!\n");
> +		return;
>  	}
> -
> -	return 0;
>  }
> /*
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h  
> b/drivers/gpu/drm/i915/intel_guc_fw.h
> index 023f5ba..7f6ccaf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h
> @@ -27,7 +27,7 @@
> struct intel_guc;
> -int intel_guc_fw_select(struct intel_guc *guc);
> +void intel_guc_fw_select(struct intel_guc *guc);
>  int intel_guc_fw_upload(struct intel_guc *guc);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> b/drivers/gpu/drm/i915/intel_huc.c
> index c8a48cb..4e700ab 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -108,7 +108,8 @@ void intel_huc_select_fw(struct intel_huc *huc)
>  		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>  		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>  	} else {
> -		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> +		if (HAS_GUC(dev_priv))
> +			DRM_ERROR("No HuC FW known for platform with HuC!\n");
>  		return;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 9369ade..dc978a0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -82,11 +82,17 @@ void intel_uc_sanitize_options(struct  
> drm_i915_private *dev_priv)
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
> +	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_huc *huc = &dev_priv->huc;

Please run checkpatch and add separation line here

>  	intel_guc_init_early(&dev_priv->guc);

You can now pass 'guc' as param

> +	intel_guc_fw_select(guc);
> +	intel_huc_select_fw(huc);

To avoid redundant checks in select_fw() functions we can
exit earlier with

	if (!HAS_GUC(dev_priv))
		return;

>  }
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!HAS_GUC(dev_priv))
> +		return;
>  	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>  	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/6] drm/i915/guc : Updating GuC and HuC firmware select function

[Sujaritha]

On 10/25/2017 08:56 AM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:23 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> Updating GuC and HuC firmware select function to support removing
>> i915_modparams.enable_guc_loading module parameter.
>
> Hmm, is it correct order of patches ? this modparam was already 
> removed in 2/6

I will move this to be the third patch. Since the change to uc.c was 
separated from 2/6,
I had included that as 3/6.
>
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating message unification into a separate patch
>>
>> v6: Re-factoring code (Sagar, Michal)
>>     Rebase
>>
>> v7: Separating from previuos patch (Sagar)
>>     Rebase
>>
>> v8: Including change to intel_uc.c
>>     Applying review comments (Michal)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_fw.c | 10 +++-------
>>  drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
>>  drivers/gpu/drm/i915/intel_huc.c    |  3 ++-
>>  drivers/gpu/drm/i915/intel_uc.c     |  6 ++++++
>>  4 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c 
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index ef67a36..b9f834f 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>> @@ -60,10 +60,8 @@
>>   * intel_guc_fw_select() - selects GuC firmware for uploading
>>   *
>>   * @guc:    intel_guc struct
>> - *
>> - * Return: zero when we know firmware, non-zero in other case
>>   */
>> -int intel_guc_fw_select(struct intel_guc *guc)
>> +void intel_guc_fw_select(struct intel_guc *guc)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> @@ -90,11 +88,9 @@ int intel_guc_fw_select(struct intel_guc *guc)
>>          guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>>          guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>>      } else {
>> -        DRM_ERROR("No GuC firmware known for platform with GuC!\n");
>> -        return -ENOENT;
>> +        DRM_ERROR("No GuC FW known for platform with GuC!\n");
>> +        return;
>>      }
>> -
>> -    return 0;
>>  }
>> /*
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h 
>> b/drivers/gpu/drm/i915/intel_guc_fw.h
>> index 023f5ba..7f6ccaf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h
>> @@ -27,7 +27,7 @@
>> struct intel_guc;
>> -int intel_guc_fw_select(struct intel_guc *guc);
>> +void intel_guc_fw_select(struct intel_guc *guc);
>>  int intel_guc_fw_upload(struct intel_guc *guc);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index c8a48cb..4e700ab 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -108,7 +108,8 @@ void intel_huc_select_fw(struct intel_huc *huc)
>>          huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>>          huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>>      } else {
>> -        DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>> +        if (HAS_GUC(dev_priv))
>> +            DRM_ERROR("No HuC FW known for platform with HuC!\n");
>>          return;
>>      }
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9369ade..dc978a0 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -82,11 +82,17 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>>  {
>> +    struct intel_guc *guc = &dev_priv->guc;
>> +    struct intel_huc *huc = &dev_priv->huc;
>
> Please run checkpatch and add separation line here

Will do.
>
>> intel_guc_init_early(&dev_priv->guc);
>
> You can now pass 'guc' as param

Will do.
>
>> +    intel_guc_fw_select(guc);
>> +    intel_huc_select_fw(huc);
>
> To avoid redundant checks in select_fw() functions we can
> exit earlier with

I will add the early exit condition.

Thanks for the review.
>
>     if (!HAS_GUC(dev_priv))
>         return;
>
>>  }
>> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>>  {
>> +    if (!HAS_GUC(dev_priv))
>> +        return;
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>>  }

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

[PATCH v8 5/6] drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter

[Sujaritha Sundaresan]

Replacing conditions to remove dependance on enable_guc_submission

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..c9f0167 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (!i915_modparams.enable_guc_submission ||
-	    (i915_modparams.guc_log_level < 0))
+	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission ||
-	    (i915_modparams.guc_log_level < 0))
+	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -657,7 +655,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
+	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-- 
1.9.1

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

Re: [PATCH v8 5/6] drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter

[Sagar Arun Kamble]

On 10/24/2017 10:51 PM, Sujaritha Sundaresan wrote:
> Replacing conditions to remove dependance on enable_guc_submission
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_log.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 76d3eb1..c9f0167 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
> -	if (!i915_modparams.enable_guc_submission ||
> -	    (i915_modparams.guc_log_level < 0))
> +	if (!NEEDS_GUC_FW(dev_priv))
this will just check if GuC is loaded but we will still need check for 
log_level.
>   		return;
>   
>   	/* First disable the interrupts, will be renabled afterwards */
> @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   
>   void i915_guc_log_register(struct drm_i915_private *dev_priv)
>   {
> -	if (!i915_modparams.enable_guc_submission ||
> -	    (i915_modparams.guc_log_level < 0))
> +	if (!NEEDS_GUC_FW(dev_priv))
same here.
>   		return;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -657,7 +655,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
>   
>   void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   {
> -	if (!i915_modparams.enable_guc_submission)
> +	if (!NEEDS_GUC_FW(dev_priv))
This is fine.
>   		return;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);

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

[PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission

[Sujaritha Sundaresan]

The Additional Data Struct (ADS) contains objects that are required by
guc post FW load and are not necessarily submission-only (although that's
our current only use-case). If in the future we load GuC with submission
disabled to use some other GuC feature we might still end up requiring
something inside the ADS, so it makes more sense for them to be always
created if GuC is loaded.

Similarly, we still want to access GuC logs even if GuC submission is
disable to debug issues with GuC loading or with wathever we're using
GuC for.

To make a concrete example, the pages used by GuC to save state during
suspend are allocated as part of the ADS.

v3: Group initialization of GuC objects

v2: Decoupling ADS together with logs

v3: Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating group object initialization into next patch
    Clarifying commit message

v6: Reverting to goto err format (Michal)
    Moved guc_ads functions to dedicated file
    Rebase

v7: Rebase

v8: Applying review comments (Michal)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 139 +--------------------------
 drivers/gpu/drm/i915/intel_guc_ads.c       | 149 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_ads.h       |  33 +++++++
 drivers/gpu/drm/i915/intel_uc.c            |  38 +++++++-
 5 files changed, 220 insertions(+), 140 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 6c3b048..d7ce07e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
 i915-y += intel_uc.o \
 	  intel_uc_fw.o \
 	  intel_guc.o \
+	  intel_guc_ads.o \
 	  intel_guc_ct.o \
 	  intel_guc_log.o \
 	  intel_guc_fw.o \
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114..3a56429 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -72,13 +72,6 @@
  * ELSP context descriptor dword into Work Item.
  * See guc_wq_item_append()
  *
- * ADS:
- * The Additional Data Struct (ADS) has pointers for different buffers used by
- * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
- * scheduling policies (guc_policies), a structure describing a collection of
- * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
- * its internal state for sleep.
- *
  */
 
 static inline bool is_high_priority(struct i915_guc_client* client)
@@ -855,115 +848,6 @@ static void guc_client_free(struct i915_guc_client *client)
 	kfree(client);
 }
 
-static void guc_policy_init(struct guc_policy *policy)
-{
-	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
-	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
-	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
-	policy->policy_flags = 0;
-}
-
-static void guc_policies_init(struct guc_policies *policies)
-{
-	struct guc_policy *policy;
-	u32 p, i;
-
-	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
-	policies->max_num_work_items = POLICY_MAX_NUM_WI;
-
-	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
-		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
-			policy = &policies->policy[p][i];
-
-			guc_policy_init(policy);
-		}
-	}
-
-	policies->is_valid = 1;
-}
-
-/*
- * The first 80 dwords of the register state context, containing the
- * execlists and ppgtt registers.
- */
-#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
-
-static int guc_ads_create(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct i915_vma *vma;
-	struct page *page;
-	/* The ads obj includes the struct itself and buffers passed to GuC */
-	struct {
-		struct guc_ads ads;
-		struct guc_policies policies;
-		struct guc_mmio_reg_state reg_state;
-		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
-	} __packed *blob;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
-	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
-	u32 base;
-
-	GEM_BUG_ON(guc->ads_vma);
-
-	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
-
-	guc->ads_vma = vma;
-
-	page = i915_vma_first_page(vma);
-	blob = kmap(page);
-
-	/* GuC scheduling policies */
-	guc_policies_init(&blob->policies);
-
-	/* MMIO reg state */
-	for_each_engine(engine, dev_priv, id) {
-		blob->reg_state.white_list[engine->guc_id].mmio_start =
-			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
-
-		/* Nothing to be saved or restored for now. */
-		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;
-
-	/*
-	 * The GuC expects us to exclude the portion of the context image that
-	 * it skips from the size it is to read. It starts reading from after
-	 * the execlist context (so skipping the first page [PPHWSP] and 80
-	 * dwords). Weird guc is weird.
-	 */
-	for_each_engine(engine, dev_priv, id)
-		blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size;
-
-	base = guc_ggtt_offset(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);
-
-	kunmap(page);
-
-	return 0;
-}
-
-static void guc_ads_destroy(struct intel_guc *guc)
-{
-	i915_vma_unpin_and_release(&guc->ads_vma);
-}
-
 /*
  * Set up the memory resources to be shared with the GuC (via the GGTT)
  * at firmware loading time.
@@ -973,7 +857,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
 	void *vaddr;
-	int ret;
 
 	if (guc->stage_desc_pool)
 		return 0;
@@ -988,31 +871,15 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
 	if (IS_ERR(vaddr)) {
-		ret = PTR_ERR(vaddr);
-		goto err_vma;
+		i915_vma_unpin_and_release(&guc->stage_desc_pool);
+		return PTR_ERR(vaddr);
 	}
 
 	guc->stage_desc_pool_vaddr = vaddr;
 
-	ret = intel_guc_log_create(guc);
-	if (ret < 0)
-		goto err_vaddr;
-
-	ret = guc_ads_create(guc);
-	if (ret < 0)
-		goto err_log;
-
 	ida_init(&guc->stage_ids);
 
 	return 0;
-
-err_log:
-	intel_guc_log_destroy(guc);
-err_vaddr:
-	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
-err_vma:
-	i915_vma_unpin_and_release(&guc->stage_desc_pool);
-	return ret;
 }
 
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
@@ -1020,8 +887,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	ida_destroy(&guc->stage_ids);
-	guc_ads_destroy(guc);
-	intel_guc_log_destroy(guc);
 	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
 	i915_vma_unpin_and_release(&guc->stage_desc_pool);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
new file mode 100644
index 0000000..bff93fd
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -0,0 +1,149 @@
+/*
+ * Copyright © 2014-2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "intel_guc_ads.h"
+#include "intel_uc.h"
+#include "i915_drv.h"
+#include "i915_guc_submission.h"
+
+/**
+ * DOC: GuC Additional Data Struct
+ *
+ * ADS:
+ * The Additional Data Struct (ADS) has pointers for different buffers used by
+ * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
+ * scheduling policies (guc_policies), a structure describing a collection of
+ * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
+ * its internal state for sleep.
+ *
+ */
+
+static void guc_policy_init(struct guc_policy *policy)
+{
+    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
+    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
+    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
+    policy->policy_flags = 0;
+}
+ 
+void guc_policies_init(struct guc_policies *policies)
+{
+    struct guc_policy *policy;
+    u32 p, i;
+ 
+    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
+    policies->max_num_work_items = POLICY_MAX_NUM_WI;
+ 
+    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
+        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
+            policy = &policies->policy[p][i];
+ 
+            guc_policy_init(policy);
+        }
+    }
+ 
+    policies->is_valid = 1;
+}
+ 
+/*
+ * The first 80 dwords of the register state context, containing the
+ * execlists and ppgtt registers.
+ */
+#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
+ 
+int intel_guc_ads_create(struct intel_guc *guc)
+{
+    struct drm_i915_private *dev_priv = guc_to_i915(guc);
+    struct i915_vma *vma;
+    struct page *page;
+    /* The ads obj includes the struct itself and buffers passed to GuC */
+    struct {
+        struct guc_ads ads;
+        struct guc_policies policies;
+        struct guc_mmio_reg_state reg_state;
+        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
+    } __packed *blob;
+    struct intel_engine_cs *engine;
+    enum intel_engine_id id;
+    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
+    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
+    u32 base;
+ 
+    GEM_BUG_ON(guc->ads_vma);
+ 
+    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
+    if (IS_ERR(vma))
+        return PTR_ERR(vma);
+ 
+    guc->ads_vma = vma;
+
+    page = i915_vma_first_page(vma);
+    blob = kmap(page);
+
+    /* GuC scheduling policies */
+    guc_policies_init(&blob->policies);
+ 
+    /* MMIO reg state */
+    for_each_engine(engine, dev_priv, id) {
+        blob->reg_state.white_list[engine->guc_id].mmio_start =
+            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
+
+        /* Nothing to be saved or restored for now. */
+        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;
+
+    /*
+     * The GuC expects us to exclude the portion of the context image that
+     * it skips from the size it is to read. It starts reading from after
+     * the execlist context (so skipping the first page [PPHWSP] and 80
+     * dwords). Weird guc is weird.
+     */
+    for_each_engine(engine, dev_priv, id)
+        blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size;
+
+    base = guc_ggtt_offset(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);
+ 
+    kunmap(page);
+
+    return 0;
+}
+
+void intel_guc_ads_destroy(struct intel_guc *guc)
+{
+    i915_vma_unpin_and_release(&guc->ads_vma);
+}
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h b/drivers/gpu/drm/i915/intel_guc_ads.h
new file mode 100644
index 0000000..3ef9a5e
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#ifndef _INTEL_GUC_ADS_H_
+#define _INTEL_GUC_ADS_H_
+
+struct intel_guc;
+
+int intel_guc_ads_create(struct intel_guc *guc);
+void intel_guc_ads_destroy(struct intel_guc *guc);
+
+#endif
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index dc978a0..12db5bd 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,7 @@
 
 #include "intel_uc.h"
 #include "i915_drv.h"
+#include "intel_guc_ads.h"
 #include "i915_guc_submission.h"
 
 /* Reset GuC providing us with fresh state for both GuC and HuC.
@@ -154,6 +155,34 @@ static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
+static int guc_shared_objects_init(struct intel_guc *guc)
+{
+	int ret;
+
+	if (guc->ads_vma)
+		return 0;
+
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_logs;
+
+	ret = intel_guc_ads_create(guc);
+	if (ret < 0)
+		goto err_ads;
+
+err_ads:
+	intel_guc_ads_destroy(guc);
+err_logs:
+	intel_guc_log_destroy(guc);
+	return ret;
+}
+
+static void guc_shared_objects_fini(struct intel_guc *guc)
+{
+	intel_guc_ads_destroy(guc);
+	intel_guc_log_destroy(guc);
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -168,6 +197,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
+	ret = guc_shared_objects_init(guc);
+
 	if (i915_modparams.enable_guc_submission) {
 		/*
 		 * This is stuff we need to have available at fw load time
@@ -175,7 +206,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		 */
 		ret = i915_guc_submission_init(dev_priv);
 		if (ret)
-			goto err_guc;
+			goto err_shared;
 	}
 
 	/* init WOPCM */
@@ -252,8 +283,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_submission:
 	if (i915_modparams.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
-err_guc:
-	i915_ggtt_disable_guc(dev_priv);
+err_shared:
+	guc_shared_objects_fini(guc);
 
 	if (i915_modparams.enable_guc_submission > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
@@ -285,5 +316,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		i915_guc_submission_fini(dev_priv);
 	}
 
+	guc_shared_objects_fini(&dev_priv->guc);
 	i915_ggtt_disable_guc(dev_priv);
 }
-- 
1.9.1

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

Re: [PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission

[Michal Wajdeczko]

On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> The Additional Data Struct (ADS) contains objects that are required by
> guc post FW load and are not necessarily submission-only (although that's
> our current only use-case). If in the future we load GuC with submission
> disabled to use some other GuC feature we might still end up requiring
> something inside the ADS, so it makes more sense for them to be always
> created if GuC is loaded.
>
> Similarly, we still want to access GuC logs even if GuC submission is
> disable to debug issues with GuC loading or with wathever we're using
> GuC for.
>
> To make a concrete example, the pages used by GuC to save state during
> suspend are allocated as part of the ADS.
>
> v3: Group initialization of GuC objects
>
> v2: Decoupling ADS together with logs
>
> v3: Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating group object initialization into next patch
>     Clarifying commit message
>
> v6: Reverting to goto err format (Michal)
>     Moved guc_ads functions to dedicated file
>     Rebase
>
> v7: Rebase
>
> v8: Applying review comments (Michal)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_guc_submission.c | 139  
> +--------------------------
>  drivers/gpu/drm/i915/intel_guc_ads.c       | 149  
> +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_ads.h       |  33 +++++++
>  drivers/gpu/drm/i915/intel_uc.c            |  38 +++++++-
>  5 files changed, 220 insertions(+), 140 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile  
> b/drivers/gpu/drm/i915/Makefile
> index 6c3b048..d7ce07e 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
>  i915-y += intel_uc.o \
>  	  intel_uc_fw.o \
>  	  intel_guc.o \
> +	  intel_guc_ads.o \
>  	  intel_guc_ct.o \
>  	  intel_guc_log.o \
>  	  intel_guc_fw.o \
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c  
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114..3a56429 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -72,13 +72,6 @@
>   * ELSP context descriptor dword into Work Item.
>   * See guc_wq_item_append()
>   *
> - * ADS:
> - * The Additional Data Struct (ADS) has pointers for different buffers  
> used by
> - * the GuC. One single gem object contains the ADS struct itself  
> (guc_ads), the
> - * scheduling policies (guc_policies), a structure describing a  
> collection of
> - * register sets (guc_mmio_reg_state) and some extra pages for the GuC  
> to save
> - * its internal state for sleep.
> - *
>   */
> static inline bool is_high_priority(struct i915_guc_client* client)
> @@ -855,115 +848,6 @@ static void guc_client_free(struct i915_guc_client  
> *client)
>  	kfree(client);
>  }
> -static void guc_policy_init(struct guc_policy *policy)
> -{
> -	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> -	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
> -	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
> -	policy->policy_flags = 0;
> -}
> -
> -static void guc_policies_init(struct guc_policies *policies)
> -{
> -	struct guc_policy *policy;
> -	u32 p, i;
> -
> -	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
> -	policies->max_num_work_items = POLICY_MAX_NUM_WI;
> -
> -	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
> -		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> -			policy = &policies->policy[p][i];
> -
> -			guc_policy_init(policy);
> -		}
> -	}
> -
> -	policies->is_valid = 1;
> -}
> -
> -/*
> - * The first 80 dwords of the register state context, containing the
> - * execlists and ppgtt registers.
> - */
> -#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
> -
> -static int guc_ads_create(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct i915_vma *vma;
> -	struct page *page;
> -	/* The ads obj includes the struct itself and buffers passed to GuC */
> -	struct {
> -		struct guc_ads ads;
> -		struct guc_policies policies;
> -		struct guc_mmio_reg_state reg_state;
> -		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> -	} __packed *blob;
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
> -	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +  
> LR_HW_CONTEXT_SIZE;
> -	u32 base;
> -
> -	GEM_BUG_ON(guc->ads_vma);
> -
> -	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
> -	if (IS_ERR(vma))
> -		return PTR_ERR(vma);
> -
> -	guc->ads_vma = vma;
> -
> -	page = i915_vma_first_page(vma);
> -	blob = kmap(page);
> -
> -	/* GuC scheduling policies */
> -	guc_policies_init(&blob->policies);
> -
> -	/* MMIO reg state */
> -	for_each_engine(engine, dev_priv, id) {
> -		blob->reg_state.white_list[engine->guc_id].mmio_start =
> -			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> -
> -		/* Nothing to be saved or restored for now. */
> -		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;
> -
> -	/*
> -	 * The GuC expects us to exclude the portion of the context image that
> -	 * it skips from the size it is to read. It starts reading from after
> -	 * the execlist context (so skipping the first page [PPHWSP] and 80
> -	 * dwords). Weird guc is weird.
> -	 */
> -	for_each_engine(engine, dev_priv, id)
> -		blob->ads.eng_state_size[engine->guc_id] = engine->context_size -  
> skipped_size;
> -
> -	base = guc_ggtt_offset(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);
> -
> -	kunmap(page);
> -
> -	return 0;
> -}
> -
> -static void guc_ads_destroy(struct intel_guc *guc)
> -{
> -	i915_vma_unpin_and_release(&guc->ads_vma);
> -}
> -
>  /*
>   * Set up the memory resources to be shared with the GuC (via the GGTT)
>   * at firmware loading time.
> @@ -973,7 +857,6 @@ int i915_guc_submission_init(struct drm_i915_private  
> *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
>  	struct i915_vma *vma;
>  	void *vaddr;
> -	int ret;
> 	if (guc->stage_desc_pool)
>  		return 0;
> @@ -988,31 +871,15 @@ int i915_guc_submission_init(struct  
> drm_i915_private *dev_priv)
> 	vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
>  	if (IS_ERR(vaddr)) {
> -		ret = PTR_ERR(vaddr);
> -		goto err_vma;
> +		i915_vma_unpin_and_release(&guc->stage_desc_pool);
> +		return PTR_ERR(vaddr);
>  	}
> 	guc->stage_desc_pool_vaddr = vaddr;
> -	ret = intel_guc_log_create(guc);
> -	if (ret < 0)
> -		goto err_vaddr;
> -
> -	ret = guc_ads_create(guc);
> -	if (ret < 0)
> -		goto err_log;
> -
>  	ida_init(&guc->stage_ids);
> 	return 0;
> -
> -err_log:
> -	intel_guc_log_destroy(guc);
> -err_vaddr:
> -	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> -err_vma:
> -	i915_vma_unpin_and_release(&guc->stage_desc_pool);
> -	return ret;
>  }
> void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> @@ -1020,8 +887,6 @@ void i915_guc_submission_fini(struct  
> drm_i915_private *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
> 	ida_destroy(&guc->stage_ids);
> -	guc_ads_destroy(guc);
> -	intel_guc_log_destroy(guc);
>  	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>  	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c  
> b/drivers/gpu/drm/i915/intel_guc_ads.c
> new file mode 100644
> index 0000000..bff93fd
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -0,0 +1,149 @@
> +/*
> + * Copyright © 2014-2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person  
> obtaining a
> + * copy of this software and associated documentation files (the  
> "Software"),
> + * to deal in the Software without restriction, including without  
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,  
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the  
> next
> + * paragraph) shall be included in all copies or substantial portions  
> of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,  
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF  
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT  
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR  
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,  
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER  
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "intel_guc_ads.h"
> +#include "intel_uc.h"
> +#include "i915_drv.h"
> +#include "i915_guc_submission.h"
> +
> +/**
> + * DOC: GuC Additional Data Struct
> + *
> + * ADS:
> + * The Additional Data Struct (ADS) has pointers for different buffers  
> used by
> + * the GuC. One single gem object contains the ADS struct itself  
> (guc_ads), the
> + * scheduling policies (guc_policies), a structure describing a  
> collection of
> + * register sets (guc_mmio_reg_state) and some extra pages for the GuC  
> to save
> + * its internal state for sleep.
> + *
> + */
> +
> +static void guc_policy_init(struct guc_policy *policy)
> +{
> +    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> +    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
> +    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
> +    policy->policy_flags = 0;
> +}
> +
> +void guc_policies_init(struct guc_policies *policies)
> +{
> +    struct guc_policy *policy;
> +    u32 p, i;
> +
> +    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
> +    policies->max_num_work_items = POLICY_MAX_NUM_WI;
> +
> +    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
> +        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> +            policy = &policies->policy[p][i];
> +
> +            guc_policy_init(policy);
> +        }
> +    }
> +
> +    policies->is_valid = 1;
> +}
> +
> +/*
> + * The first 80 dwords of the register state context, containing the
> + * execlists and ppgtt registers.
> + */
> +#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
> +
> +int intel_guc_ads_create(struct intel_guc *guc)
> +{
> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +    struct i915_vma *vma;
> +    struct page *page;
> +    /* The ads obj includes the struct itself and buffers passed to GuC  
> */
> +    struct {
> +        struct guc_ads ads;
> +        struct guc_policies policies;
> +        struct guc_mmio_reg_state reg_state;
> +        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> +    } __packed *blob;
> +    struct intel_engine_cs *engine;
> +    enum intel_engine_id id;
> +    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
> +    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +  
> LR_HW_CONTEXT_SIZE;
> +    u32 base;
> +
> +    GEM_BUG_ON(guc->ads_vma);
> +
> +    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
> +    if (IS_ERR(vma))
> +        return PTR_ERR(vma);
> +
> +    guc->ads_vma = vma;
> +
> +    page = i915_vma_first_page(vma);
> +    blob = kmap(page);
> +
> +    /* GuC scheduling policies */
> +    guc_policies_init(&blob->policies);
> +
> +    /* MMIO reg state */
> +    for_each_engine(engine, dev_priv, id) {
> +        blob->reg_state.white_list[engine->guc_id].mmio_start =
> +            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> +
> +        /* Nothing to be saved or restored for now. */
> +        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;
> +
> +    /*
> +     * The GuC expects us to exclude the portion of the context image  
> that
> +     * it skips from the size it is to read. It starts reading from  
> after
> +     * the execlist context (so skipping the first page [PPHWSP] and 80
> +     * dwords). Weird guc is weird.
> +     */
> +    for_each_engine(engine, dev_priv, id)
> +        blob->ads.eng_state_size[engine->guc_id] = engine->context_size  
> - skipped_size;
> +
> +    base = guc_ggtt_offset(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);
> +
> +    kunmap(page);
> +
> +    return 0;
> +}
> +
> +void intel_guc_ads_destroy(struct intel_guc *guc)
> +{
> +    i915_vma_unpin_and_release(&guc->ads_vma);
> +}
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h  
> b/drivers/gpu/drm/i915/intel_guc_ads.h
> new file mode 100644
> index 0000000..3ef9a5e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person  
> obtaining a
> + * copy of this software and associated documentation files (the  
> "Software"),
> + * to deal in the Software without restriction, including without  
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,  
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the  
> next
> + * paragraph) shall be included in all copies or substantial portions  
> of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,  
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF  
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT  
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR  
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,  
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER  
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef _INTEL_GUC_ADS_H_
> +#define _INTEL_GUC_ADS_H_
> +
> +struct intel_guc;
> +
> +int intel_guc_ads_create(struct intel_guc *guc);
> +void intel_guc_ads_destroy(struct intel_guc *guc);
> +
> +#endif
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index dc978a0..12db5bd 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -24,6 +24,7 @@
> #include "intel_uc.h"
>  #include "i915_drv.h"
> +#include "intel_guc_ads.h"
>  #include "i915_guc_submission.h"
> /* Reset GuC providing us with fresh state for both GuC and HuC.
> @@ -154,6 +155,34 @@ static void guc_disable_communication(struct  
> intel_guc *guc)
>  	guc->send = intel_guc_send_nop;
>  }
> +static int guc_shared_objects_init(struct intel_guc *guc)
> +{
> +	int ret;
> +
> +	if (guc->ads_vma)
> +		return 0;

Hmm, this 'ads' internal member, so maybe this check should
be moved to intel_guc_ads_create() ?

> +
> +	ret = intel_guc_log_create(guc);
> +	if (ret < 0)
> +		goto err_logs;

Hmm, if intel_guc_log_create() failed then there should be
no reason to call intel_guc_log_destroy()

> +
> +	ret = intel_guc_ads_create(guc);
> +	if (ret < 0)
> +		goto err_ads;
> +
> +err_ads:
> +	intel_guc_ads_destroy(guc);
> +err_logs:
> +	intel_guc_log_destroy(guc);
> +	return ret;
> +}
> +
> +static void guc_shared_objects_fini(struct intel_guc *guc)
> +{
> +	intel_guc_ads_destroy(guc);
> +	intel_guc_log_destroy(guc);
> +}
> +
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -168,6 +197,8 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
> +	ret = guc_shared_objects_init(guc);

If this fails, does it make sense to continue ?

> +
>  	if (i915_modparams.enable_guc_submission) {
>  		/*
>  		 * This is stuff we need to have available at fw load time
> @@ -175,7 +206,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  		 */
>  		ret = i915_guc_submission_init(dev_priv);
>  		if (ret)
> -			goto err_guc;
> +			goto err_shared;
>  	}
> 	/* init WOPCM */
> @@ -252,8 +283,8 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  err_submission:
>  	if (i915_modparams.enable_guc_submission)
>  		i915_guc_submission_fini(dev_priv);
> -err_guc:
> -	i915_ggtt_disable_guc(dev_priv);
> +err_shared:
> +	guc_shared_objects_fini(guc);
> 	if (i915_modparams.enable_guc_submission > 1) {
>  		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> @@ -285,5 +316,6 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  		i915_guc_submission_fini(dev_priv);
>  	}
> +	guc_shared_objects_fini(&dev_priv->guc);
>  	i915_ggtt_disable_guc(dev_priv);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission

[Sagar Arun Kamble]

On 10/25/2017 9:49 PM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> The Additional Data Struct (ADS) contains objects that are required by
>> guc post FW load and are not necessarily submission-only (although 
>> that's
>> our current only use-case). If in the future we load GuC with submission
>> disabled to use some other GuC feature we might still end up requiring
>> something inside the ADS, so it makes more sense for them to be always
>> created if GuC is loaded.
>>
>> Similarly, we still want to access GuC logs even if GuC submission is
>> disable to debug issues with GuC loading or with wathever we're using
>> GuC for.
>>
>> To make a concrete example, the pages used by GuC to save state during
>> suspend are allocated as part of the ADS.
>>
>> v3: Group initialization of GuC objects
>>
>> v2: Decoupling ADS together with logs
>>
>> v3: Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating group object initialization into next patch
>>     Clarifying commit message
>>
>> v6: Reverting to goto err format (Michal)
>>     Moved guc_ads functions to dedicated file
>>     Rebase
>>
>> v7: Rebase
>>
>> v8: Applying review comments (Michal)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile              |   1 +
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 139 
>> +--------------------------
>>  drivers/gpu/drm/i915/intel_guc_ads.c       | 149 
>> +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_guc_ads.h       |  33 +++++++
>>  drivers/gpu/drm/i915/intel_uc.c            |  38 +++++++-
>>  5 files changed, 220 insertions(+), 140 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
>>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile 
>> b/drivers/gpu/drm/i915/Makefile
>> index 6c3b048..d7ce07e 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
>>  i915-y += intel_uc.o \
>>        intel_uc_fw.o \
>>        intel_guc.o \
>> +      intel_guc_ads.o \
>>        intel_guc_ct.o \
>>        intel_guc_log.o \
>>        intel_guc_fw.o \
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a2e8114..3a56429 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -72,13 +72,6 @@
>>   * ELSP context descriptor dword into Work Item.
>>   * See guc_wq_item_append()
>>   *
>> - * ADS:
>> - * The Additional Data Struct (ADS) has pointers for different 
>> buffers used by
>> - * the GuC. One single gem object contains the ADS struct itself 
>> (guc_ads), the
>> - * scheduling policies (guc_policies), a structure describing a 
>> collection of
>> - * register sets (guc_mmio_reg_state) and some extra pages for the 
>> GuC to save
>> - * its internal state for sleep.
>> - *
>>   */
>> static inline bool is_high_priority(struct i915_guc_client* client)
>> @@ -855,115 +848,6 @@ static void guc_client_free(struct 
>> i915_guc_client *client)
>>      kfree(client);
>>  }
>> -static void guc_policy_init(struct guc_policy *policy)
>> -{
>> -    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>> -    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>> -    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>> -    policy->policy_flags = 0;
>> -}
>> -
>> -static void guc_policies_init(struct guc_policies *policies)
>> -{
>> -    struct guc_policy *policy;
>> -    u32 p, i;
>> -
>> -    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>> -    policies->max_num_work_items = POLICY_MAX_NUM_WI;
>> -
>> -    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>> -        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>> -            policy = &policies->policy[p][i];
>> -
>> -            guc_policy_init(policy);
>> -        }
>> -    }
>> -
>> -    policies->is_valid = 1;
>> -}
>> -
>> -/*
>> - * The first 80 dwords of the register state context, containing the
>> - * execlists and ppgtt registers.
>> - */
>> -#define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>> -
>> -static int guc_ads_create(struct intel_guc *guc)
>> -{
>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> -    struct i915_vma *vma;
>> -    struct page *page;
>> -    /* The ads obj includes the struct itself and buffers passed to 
>> GuC */
>> -    struct {
>> -        struct guc_ads ads;
>> -        struct guc_policies policies;
>> -        struct guc_mmio_reg_state reg_state;
>> -        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>> -    } __packed *blob;
>> -    struct intel_engine_cs *engine;
>> -    enum intel_engine_id id;
>> -    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>> -    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + 
>> LR_HW_CONTEXT_SIZE;
>> -    u32 base;
>> -
>> -    GEM_BUG_ON(guc->ads_vma);
>> -
>> -    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>> -    if (IS_ERR(vma))
>> -        return PTR_ERR(vma);
>> -
>> -    guc->ads_vma = vma;
>> -
>> -    page = i915_vma_first_page(vma);
>> -    blob = kmap(page);
>> -
>> -    /* GuC scheduling policies */
>> -    guc_policies_init(&blob->policies);
>> -
>> -    /* MMIO reg state */
>> -    for_each_engine(engine, dev_priv, id) {
>> - blob->reg_state.white_list[engine->guc_id].mmio_start =
>> -            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>> -
>> -        /* Nothing to be saved or restored for now. */
>> -        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;
>> -
>> -    /*
>> -     * The GuC expects us to exclude the portion of the context 
>> image that
>> -     * it skips from the size it is to read. It starts reading from 
>> after
>> -     * the execlist context (so skipping the first page [PPHWSP] and 80
>> -     * dwords). Weird guc is weird.
>> -     */
>> -    for_each_engine(engine, dev_priv, id)
>> -        blob->ads.eng_state_size[engine->guc_id] = 
>> engine->context_size - skipped_size;
>> -
>> -    base = guc_ggtt_offset(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);
>> -
>> -    kunmap(page);
>> -
>> -    return 0;
>> -}
>> -
>> -static void guc_ads_destroy(struct intel_guc *guc)
>> -{
>> -    i915_vma_unpin_and_release(&guc->ads_vma);
>> -}
>> -
>>  /*
>>   * Set up the memory resources to be shared with the GuC (via the GGTT)
>>   * at firmware loading time.
>> @@ -973,7 +857,6 @@ int i915_guc_submission_init(struct 
>> drm_i915_private *dev_priv)
>>      struct intel_guc *guc = &dev_priv->guc;
>>      struct i915_vma *vma;
>>      void *vaddr;
>> -    int ret;
>>     if (guc->stage_desc_pool)
>>          return 0;
>> @@ -988,31 +871,15 @@ int i915_guc_submission_init(struct 
>> drm_i915_private *dev_priv)
>>     vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, 
>> I915_MAP_WB);
>>      if (IS_ERR(vaddr)) {
>> -        ret = PTR_ERR(vaddr);
>> -        goto err_vma;
>> + i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> +        return PTR_ERR(vaddr);
>>      }
>>     guc->stage_desc_pool_vaddr = vaddr;
>> -    ret = intel_guc_log_create(guc);
>> -    if (ret < 0)
>> -        goto err_vaddr;
>> -
>> -    ret = guc_ads_create(guc);
>> -    if (ret < 0)
>> -        goto err_log;
>> -
>>      ida_init(&guc->stage_ids);
>>     return 0;
>> -
>> -err_log:
>> -    intel_guc_log_destroy(guc);
>> -err_vaddr:
>> -    i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> -err_vma:
>> -    i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> -    return ret;
>>  }
>> void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>> @@ -1020,8 +887,6 @@ void i915_guc_submission_fini(struct 
>> drm_i915_private *dev_priv)
>>      struct intel_guc *guc = &dev_priv->guc;
>>     ida_destroy(&guc->stage_ids);
>> -    guc_ads_destroy(guc);
>> -    intel_guc_log_destroy(guc);
>>      i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>      i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c 
>> b/drivers/gpu/drm/i915/intel_guc_ads.c
>> new file mode 100644
>> index 0000000..bff93fd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * Copyright © 2014-2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "intel_guc_ads.h"
>> +#include "intel_uc.h"
>> +#include "i915_drv.h"
>> +#include "i915_guc_submission.h"
>> +
>> +/**
>> + * DOC: GuC Additional Data Struct
>> + *
>> + * ADS:
>> + * The Additional Data Struct (ADS) has pointers for different 
>> buffers used by
>> + * the GuC. One single gem object contains the ADS struct itself 
>> (guc_ads), the
>> + * scheduling policies (guc_policies), a structure describing a 
>> collection of
>> + * register sets (guc_mmio_reg_state) and some extra pages for the 
>> GuC to save
>> + * its internal state for sleep.
>> + *
>> + */
>> +
>> +static void guc_policy_init(struct guc_policy *policy)
>> +{
>> +    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>> +    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>> +    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>> +    policy->policy_flags = 0;
>> +}
>> +
>> +void guc_policies_init(struct guc_policies *policies)
>> +{
>> +    struct guc_policy *policy;
>> +    u32 p, i;
>> +
>> +    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>> +    policies->max_num_work_items = POLICY_MAX_NUM_WI;
>> +
>> +    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>> +        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>> +            policy = &policies->policy[p][i];
>> +
>> +            guc_policy_init(policy);
>> +        }
>> +    }
>> +
>> +    policies->is_valid = 1;
>> +}
>> +
>> +/*
>> + * The first 80 dwords of the register state context, containing the
>> + * execlists and ppgtt registers.
>> + */
>> +#define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>> +
>> +int intel_guc_ads_create(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    struct i915_vma *vma;
>> +    struct page *page;
>> +    /* The ads obj includes the struct itself and buffers passed to 
>> GuC */
>> +    struct {
>> +        struct guc_ads ads;
>> +        struct guc_policies policies;
>> +        struct guc_mmio_reg_state reg_state;
>> +        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>> +    } __packed *blob;
>> +    struct intel_engine_cs *engine;
>> +    enum intel_engine_id id;
>> +    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>> +    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + 
>> LR_HW_CONTEXT_SIZE;
>> +    u32 base;
>> +
>> +    GEM_BUG_ON(guc->ads_vma);
>> +
>> +    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>> +    if (IS_ERR(vma))
>> +        return PTR_ERR(vma);
>> +
>> +    guc->ads_vma = vma;
>> +
>> +    page = i915_vma_first_page(vma);
>> +    blob = kmap(page);
>> +
>> +    /* GuC scheduling policies */
>> +    guc_policies_init(&blob->policies);
>> +
>> +    /* MMIO reg state */
>> +    for_each_engine(engine, dev_priv, id) {
>> + blob->reg_state.white_list[engine->guc_id].mmio_start =
>> +            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>> +
>> +        /* Nothing to be saved or restored for now. */
>> +        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;
>> +
>> +    /*
>> +     * The GuC expects us to exclude the portion of the context 
>> image that
>> +     * it skips from the size it is to read. It starts reading from 
>> after
>> +     * the execlist context (so skipping the first page [PPHWSP] and 80
>> +     * dwords). Weird guc is weird.
>> +     */
>> +    for_each_engine(engine, dev_priv, id)
>> +        blob->ads.eng_state_size[engine->guc_id] = 
>> engine->context_size - skipped_size;
>> +
>> +    base = guc_ggtt_offset(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);
>> +
>> +    kunmap(page);
>> +
>> +    return 0;
>> +}
>> +
>> +void intel_guc_ads_destroy(struct intel_guc *guc)
>> +{
>> +    i915_vma_unpin_and_release(&guc->ads_vma);
>> +}
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h 
>> b/drivers/gpu/drm/i915/intel_guc_ads.h
>> new file mode 100644
>> index 0000000..3ef9a5e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef _INTEL_GUC_ADS_H_
>> +#define _INTEL_GUC_ADS_H_
>> +
>> +struct intel_guc;
>> +
>> +int intel_guc_ads_create(struct intel_guc *guc);
>> +void intel_guc_ads_destroy(struct intel_guc *guc);
>> +
>> +#endif
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index dc978a0..12db5bd 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -24,6 +24,7 @@
>> #include "intel_uc.h"
>>  #include "i915_drv.h"
>> +#include "intel_guc_ads.h"
>>  #include "i915_guc_submission.h"
>> /* Reset GuC providing us with fresh state for both GuC and HuC.
>> @@ -154,6 +155,34 @@ static void guc_disable_communication(struct 
>> intel_guc *guc)
>>      guc->send = intel_guc_send_nop;
>>  }
>> +static int guc_shared_objects_init(struct intel_guc *guc)
>> +{
>> +    int ret;
>> +
>> +    if (guc->ads_vma)
>> +        return 0;
>
> Hmm, this 'ads' internal member, so maybe this check should
> be moved to intel_guc_ads_create() ?
>
>> +
>> +    ret = intel_guc_log_create(guc);
>> +    if (ret < 0)
>> +        goto err_logs;
>
> Hmm, if intel_guc_log_create() failed then there should be
> no reason to call intel_guc_log_destroy()
>
>> +
>> +    ret = intel_guc_ads_create(guc);
>> +    if (ret < 0)
>> +        goto err_ads;
>> +
>> +err_ads:
>> +    intel_guc_ads_destroy(guc);
>> +err_logs:
>> +    intel_guc_log_destroy(guc);
>> +    return ret;
>> +}
>> +
>> +static void guc_shared_objects_fini(struct intel_guc *guc)
>> +{
>> +    intel_guc_ads_destroy(guc);
>> +    intel_guc_log_destroy(guc);
>> +}
>> +
>>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>  {
>>      struct intel_guc *guc = &dev_priv->guc;
>> @@ -168,6 +197,8 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      /* We need to notify the guc whenever we change the GGTT */
>>      i915_ggtt_enable_guc(dev_priv);
>> +    ret = guc_shared_objects_init(guc);
>
> If this fails, does it make sense to continue ?
If log creation fails I don't see a reason to not init GuC and also we 
can plan to split the log creation
into log vma and log runtime handling so I think it would be good if we 
handle log and ads separately and not
together as shared_objects.
Another improvement can be to move this init out of intel_uc_init_hw 
into i915_gem_init (and remove the stage_desc_pool check
GEM_BUG_ON in ads/log create?)
That way it is also clear that these structures are allocated only once.
>
>> +
>>      if (i915_modparams.enable_guc_submission) {
>>          /*
>>           * This is stuff we need to have available at fw load time
>> @@ -175,7 +206,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>           */
>>          ret = i915_guc_submission_init(dev_priv);
>>          if (ret)
>> -            goto err_guc;
>> +            goto err_shared;
>>      }
>>     /* init WOPCM */
>> @@ -252,8 +283,8 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>  err_submission:
>>      if (i915_modparams.enable_guc_submission)
>>          i915_guc_submission_fini(dev_priv);
>> -err_guc:
>> -    i915_ggtt_disable_guc(dev_priv);
>> +err_shared:
>> +    guc_shared_objects_fini(guc);
>>     if (i915_modparams.enable_guc_submission > 1) {
>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>> @@ -285,5 +316,6 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>          i915_guc_submission_fini(dev_priv);
>>      }
>> +    guc_shared_objects_fini(&dev_priv->guc);
>>      i915_ggtt_disable_guc(dev_priv);
>>  }

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

Re: [PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission

[Sujaritha]

On 11/02/2017 02:23 AM, Sagar Arun Kamble wrote:
>
>
> On 10/25/2017 9:49 PM, Michal Wajdeczko wrote:
>> On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan 
>> <sujaritha.sundaresan@intel.com> wrote:
>>
>>> The Additional Data Struct (ADS) contains objects that are required by
>>> guc post FW load and are not necessarily submission-only (although 
>>> that's
>>> our current only use-case). If in the future we load GuC with 
>>> submission
>>> disabled to use some other GuC feature we might still end up requiring
>>> something inside the ADS, so it makes more sense for them to be always
>>> created if GuC is loaded.
>>>
>>> Similarly, we still want to access GuC logs even if GuC submission is
>>> disable to debug issues with GuC loading or with wathever we're using
>>> GuC for.
>>>
>>> To make a concrete example, the pages used by GuC to save state during
>>> suspend are allocated as part of the ADS.
>>>
>>> v3: Group initialization of GuC objects
>>>
>>> v2: Decoupling ADS together with logs
>>>
>>> v3: Re-factoring code as per review (Michal)
>>>
>>> v4: Rebase
>>>
>>> v5: Separating group object initialization into next patch
>>>     Clarifying commit message
>>>
>>> v6: Reverting to goto err format (Michal)
>>>     Moved guc_ads functions to dedicated file
>>>     Rebase
>>>
>>> v7: Rebase
>>>
>>> v8: Applying review comments (Michal)
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/Makefile              |   1 +
>>>  drivers/gpu/drm/i915/i915_guc_submission.c | 139 
>>> +--------------------------
>>>  drivers/gpu/drm/i915/intel_guc_ads.c       | 149 
>>> +++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_guc_ads.h       |  33 +++++++
>>>  drivers/gpu/drm/i915/intel_uc.c            |  38 +++++++-
>>>  5 files changed, 220 insertions(+), 140 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
>>>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/Makefile 
>>> b/drivers/gpu/drm/i915/Makefile
>>> index 6c3b048..d7ce07e 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
>>>  i915-y += intel_uc.o \
>>>        intel_uc_fw.o \
>>>        intel_guc.o \
>>> +      intel_guc_ads.o \
>>>        intel_guc_ct.o \
>>>        intel_guc_log.o \
>>>        intel_guc_fw.o \
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index a2e8114..3a56429 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -72,13 +72,6 @@
>>>   * ELSP context descriptor dword into Work Item.
>>>   * See guc_wq_item_append()
>>>   *
>>> - * ADS:
>>> - * The Additional Data Struct (ADS) has pointers for different 
>>> buffers used by
>>> - * the GuC. One single gem object contains the ADS struct itself 
>>> (guc_ads), the
>>> - * scheduling policies (guc_policies), a structure describing a 
>>> collection of
>>> - * register sets (guc_mmio_reg_state) and some extra pages for the 
>>> GuC to save
>>> - * its internal state for sleep.
>>> - *
>>>   */
>>> static inline bool is_high_priority(struct i915_guc_client* client)
>>> @@ -855,115 +848,6 @@ static void guc_client_free(struct 
>>> i915_guc_client *client)
>>>      kfree(client);
>>>  }
>>> -static void guc_policy_init(struct guc_policy *policy)
>>> -{
>>> -    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>>> -    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>>> -    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>>> -    policy->policy_flags = 0;
>>> -}
>>> -
>>> -static void guc_policies_init(struct guc_policies *policies)
>>> -{
>>> -    struct guc_policy *policy;
>>> -    u32 p, i;
>>> -
>>> -    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>>> -    policies->max_num_work_items = POLICY_MAX_NUM_WI;
>>> -
>>> -    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>>> -        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>>> -            policy = &policies->policy[p][i];
>>> -
>>> -            guc_policy_init(policy);
>>> -        }
>>> -    }
>>> -
>>> -    policies->is_valid = 1;
>>> -}
>>> -
>>> -/*
>>> - * The first 80 dwords of the register state context, containing the
>>> - * execlists and ppgtt registers.
>>> - */
>>> -#define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>>> -
>>> -static int guc_ads_create(struct intel_guc *guc)
>>> -{
>>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> -    struct i915_vma *vma;
>>> -    struct page *page;
>>> -    /* The ads obj includes the struct itself and buffers passed to 
>>> GuC */
>>> -    struct {
>>> -        struct guc_ads ads;
>>> -        struct guc_policies policies;
>>> -        struct guc_mmio_reg_state reg_state;
>>> -        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>>> -    } __packed *blob;
>>> -    struct intel_engine_cs *engine;
>>> -    enum intel_engine_id id;
>>> -    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>>> -    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + 
>>> LR_HW_CONTEXT_SIZE;
>>> -    u32 base;
>>> -
>>> -    GEM_BUG_ON(guc->ads_vma);
>>> -
>>> -    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>>> -    if (IS_ERR(vma))
>>> -        return PTR_ERR(vma);
>>> -
>>> -    guc->ads_vma = vma;
>>> -
>>> -    page = i915_vma_first_page(vma);
>>> -    blob = kmap(page);
>>> -
>>> -    /* GuC scheduling policies */
>>> -    guc_policies_init(&blob->policies);
>>> -
>>> -    /* MMIO reg state */
>>> -    for_each_engine(engine, dev_priv, id) {
>>> - blob->reg_state.white_list[engine->guc_id].mmio_start =
>>> -            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>>> -
>>> -        /* Nothing to be saved or restored for now. */
>>> - 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;
>>> -
>>> -    /*
>>> -     * The GuC expects us to exclude the portion of the context 
>>> image that
>>> -     * it skips from the size it is to read. It starts reading from 
>>> after
>>> -     * the execlist context (so skipping the first page [PPHWSP] 
>>> and 80
>>> -     * dwords). Weird guc is weird.
>>> -     */
>>> -    for_each_engine(engine, dev_priv, id)
>>> -        blob->ads.eng_state_size[engine->guc_id] = 
>>> engine->context_size - skipped_size;
>>> -
>>> -    base = guc_ggtt_offset(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);
>>> -
>>> -    kunmap(page);
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static void guc_ads_destroy(struct intel_guc *guc)
>>> -{
>>> -    i915_vma_unpin_and_release(&guc->ads_vma);
>>> -}
>>> -
>>>  /*
>>>   * Set up the memory resources to be shared with the GuC (via the 
>>> GGTT)
>>>   * at firmware loading time.
>>> @@ -973,7 +857,6 @@ int i915_guc_submission_init(struct 
>>> drm_i915_private *dev_priv)
>>>      struct intel_guc *guc = &dev_priv->guc;
>>>      struct i915_vma *vma;
>>>      void *vaddr;
>>> -    int ret;
>>>     if (guc->stage_desc_pool)
>>>          return 0;
>>> @@ -988,31 +871,15 @@ int i915_guc_submission_init(struct 
>>> drm_i915_private *dev_priv)
>>>     vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, 
>>> I915_MAP_WB);
>>>      if (IS_ERR(vaddr)) {
>>> -        ret = PTR_ERR(vaddr);
>>> -        goto err_vma;
>>> + i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>> +        return PTR_ERR(vaddr);
>>>      }
>>>     guc->stage_desc_pool_vaddr = vaddr;
>>> -    ret = intel_guc_log_create(guc);
>>> -    if (ret < 0)
>>> -        goto err_vaddr;
>>> -
>>> -    ret = guc_ads_create(guc);
>>> -    if (ret < 0)
>>> -        goto err_log;
>>> -
>>>      ida_init(&guc->stage_ids);
>>>     return 0;
>>> -
>>> -err_log:
>>> -    intel_guc_log_destroy(guc);
>>> -err_vaddr:
>>> - i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>> -err_vma:
>>> -    i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>> -    return ret;
>>>  }
>>> void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>>> @@ -1020,8 +887,6 @@ void i915_guc_submission_fini(struct 
>>> drm_i915_private *dev_priv)
>>>      struct intel_guc *guc = &dev_priv->guc;
>>>     ida_destroy(&guc->stage_ids);
>>> -    guc_ads_destroy(guc);
>>> -    intel_guc_log_destroy(guc);
>>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>>>      i915_vma_unpin_and_release(&guc->stage_desc_pool);
>>>  }
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c 
>>> b/drivers/gpu/drm/i915/intel_guc_ads.c
>>> new file mode 100644
>>> index 0000000..bff93fd
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
>>> @@ -0,0 +1,149 @@
>>> +/*
>>> + * Copyright © 2014-2017 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to 
>>> whom the
>>> + * Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including 
>>> the next
>>> + * paragraph) shall be included in all copies or substantial 
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>>> EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, 
>>> DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>>> ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>>> OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + */
>>> +
>>> +#include "intel_guc_ads.h"
>>> +#include "intel_uc.h"
>>> +#include "i915_drv.h"
>>> +#include "i915_guc_submission.h"
>>> +
>>> +/**
>>> + * DOC: GuC Additional Data Struct
>>> + *
>>> + * ADS:
>>> + * The Additional Data Struct (ADS) has pointers for different 
>>> buffers used by
>>> + * the GuC. One single gem object contains the ADS struct itself 
>>> (guc_ads), the
>>> + * scheduling policies (guc_policies), a structure describing a 
>>> collection of
>>> + * register sets (guc_mmio_reg_state) and some extra pages for the 
>>> GuC to save
>>> + * its internal state for sleep.
>>> + *
>>> + */
>>> +
>>> +static void guc_policy_init(struct guc_policy *policy)
>>> +{
>>> +    policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>>> +    policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>>> +    policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>>> +    policy->policy_flags = 0;
>>> +}
>>> +
>>> +void guc_policies_init(struct guc_policies *policies)
>>> +{
>>> +    struct guc_policy *policy;
>>> +    u32 p, i;
>>> +
>>> +    policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>>> +    policies->max_num_work_items = POLICY_MAX_NUM_WI;
>>> +
>>> +    for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>>> +        for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>>> +            policy = &policies->policy[p][i];
>>> +
>>> +            guc_policy_init(policy);
>>> +        }
>>> +    }
>>> +
>>> +    policies->is_valid = 1;
>>> +}
>>> +
>>> +/*
>>> + * The first 80 dwords of the register state context, containing the
>>> + * execlists and ppgtt registers.
>>> + */
>>> +#define LR_HW_CONTEXT_SIZE    (80 * sizeof(u32))
>>> +
>>> +int intel_guc_ads_create(struct intel_guc *guc)
>>> +{
>>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> +    struct i915_vma *vma;
>>> +    struct page *page;
>>> +    /* The ads obj includes the struct itself and buffers passed to 
>>> GuC */
>>> +    struct {
>>> +        struct guc_ads ads;
>>> +        struct guc_policies policies;
>>> +        struct guc_mmio_reg_state reg_state;
>>> +        u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>>> +    } __packed *blob;
>>> +    struct intel_engine_cs *engine;
>>> +    enum intel_engine_id id;
>>> +    const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>>> +    const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + 
>>> LR_HW_CONTEXT_SIZE;
>>> +    u32 base;
>>> +
>>> +    GEM_BUG_ON(guc->ads_vma);
>>> +
>>> +    vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>>> +    if (IS_ERR(vma))
>>> +        return PTR_ERR(vma);
>>> +
>>> +    guc->ads_vma = vma;
>>> +
>>> +    page = i915_vma_first_page(vma);
>>> +    blob = kmap(page);
>>> +
>>> +    /* GuC scheduling policies */
>>> +    guc_policies_init(&blob->policies);
>>> +
>>> +    /* MMIO reg state */
>>> +    for_each_engine(engine, dev_priv, id) {
>>> + blob->reg_state.white_list[engine->guc_id].mmio_start =
>>> +            engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>>> +
>>> +        /* Nothing to be saved or restored for now. */
>>> + 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;
>>> +
>>> +    /*
>>> +     * The GuC expects us to exclude the portion of the context 
>>> image that
>>> +     * it skips from the size it is to read. It starts reading from 
>>> after
>>> +     * the execlist context (so skipping the first page [PPHWSP] 
>>> and 80
>>> +     * dwords). Weird guc is weird.
>>> +     */
>>> +    for_each_engine(engine, dev_priv, id)
>>> +        blob->ads.eng_state_size[engine->guc_id] = 
>>> engine->context_size - skipped_size;
>>> +
>>> +    base = guc_ggtt_offset(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);
>>> +
>>> +    kunmap(page);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void intel_guc_ads_destroy(struct intel_guc *guc)
>>> +{
>>> +    i915_vma_unpin_and_release(&guc->ads_vma);
>>> +}
>>> \ No newline at end of file
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h 
>>> b/drivers/gpu/drm/i915/intel_guc_ads.h
>>> new file mode 100644
>>> index 0000000..3ef9a5e
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
>>> @@ -0,0 +1,33 @@
>>> +/*
>>> + * Copyright © 2017 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, 
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to 
>>> whom the
>>> + * Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including 
>>> the next
>>> + * paragraph) shall be included in all copies or substantial 
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>>> EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, 
>>> DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>>> ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>>> OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + */
>>> +
>>> +#ifndef _INTEL_GUC_ADS_H_
>>> +#define _INTEL_GUC_ADS_H_
>>> +
>>> +struct intel_guc;
>>> +
>>> +int intel_guc_ads_create(struct intel_guc *guc);
>>> +void intel_guc_ads_destroy(struct intel_guc *guc);
>>> +
>>> +#endif
>>> \ No newline at end of file
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index dc978a0..12db5bd 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -24,6 +24,7 @@
>>> #include "intel_uc.h"
>>>  #include "i915_drv.h"
>>> +#include "intel_guc_ads.h"
>>>  #include "i915_guc_submission.h"
>>> /* Reset GuC providing us with fresh state for both GuC and HuC.
>>> @@ -154,6 +155,34 @@ static void guc_disable_communication(struct 
>>> intel_guc *guc)
>>>      guc->send = intel_guc_send_nop;
>>>  }
>>> +static int guc_shared_objects_init(struct intel_guc *guc)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (guc->ads_vma)
>>> +        return 0;
>>
>> Hmm, this 'ads' internal member, so maybe this check should
>> be moved to intel_guc_ads_create() ?
>>
>>> +
>>> +    ret = intel_guc_log_create(guc);
>>> +    if (ret < 0)
>>> +        goto err_logs;
>>
>> Hmm, if intel_guc_log_create() failed then there should be
>> no reason to call intel_guc_log_destroy()
>>
>>> +
>>> +    ret = intel_guc_ads_create(guc);
>>> +    if (ret < 0)
>>> +        goto err_ads;
>>> +
>>> +err_ads:
>>> +    intel_guc_ads_destroy(guc);
>>> +err_logs:
>>> +    intel_guc_log_destroy(guc);
>>> +    return ret;
>>> +}
>>> +
>>> +static void guc_shared_objects_fini(struct intel_guc *guc)
>>> +{
>>> +    intel_guc_ads_destroy(guc);
>>> +    intel_guc_log_destroy(guc);
>>> +}
>>> +
>>>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>>  {
>>>      struct intel_guc *guc = &dev_priv->guc;
>>> @@ -168,6 +197,8 @@ int intel_uc_init_hw(struct drm_i915_private 
>>> *dev_priv)
>>>      /* We need to notify the guc whenever we change the GGTT */
>>>      i915_ggtt_enable_guc(dev_priv);
>>> +    ret = guc_shared_objects_init(guc);
>>
>> If this fails, does it make sense to continue ?
> If log creation fails I don't see a reason to not init GuC and also we 
> can plan to split the log creation
> into log vma and log runtime handling so I think it would be good if 
> we handle log and ads separately and not
> together as shared_objects.
> Another improvement can be to move this init out of intel_uc_init_hw 
> into i915_gem_init (and remove the stage_desc_pool check
> GEM_BUG_ON in ads/log create?)
> That way it is also clear that these structures are allocated only once.

Yes sure, I will include these changes in the next revision. Thanks for 
the review.
>>
>>> +
>>>      if (i915_modparams.enable_guc_submission) {
>>>          /*
>>>           * This is stuff we need to have available at fw load time
>>> @@ -175,7 +206,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>>> *dev_priv)
>>>           */
>>>          ret = i915_guc_submission_init(dev_priv);
>>>          if (ret)
>>> -            goto err_guc;
>>> +            goto err_shared;
>>>      }
>>>     /* init WOPCM */
>>> @@ -252,8 +283,8 @@ int intel_uc_init_hw(struct drm_i915_private 
>>> *dev_priv)
>>>  err_submission:
>>>      if (i915_modparams.enable_guc_submission)
>>>          i915_guc_submission_fini(dev_priv);
>>> -err_guc:
>>> -    i915_ggtt_disable_guc(dev_priv);
>>> +err_shared:
>>> +    guc_shared_objects_fini(guc);
>>>     if (i915_modparams.enable_guc_submission > 1) {
>>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>>> @@ -285,5 +316,6 @@ void intel_uc_fini_hw(struct drm_i915_private 
>>> *dev_priv)
>>>          i915_guc_submission_fini(dev_priv);
>>>      }
>>> +    guc_shared_objects_fini(&dev_priv->guc);
>>>      i915_ggtt_disable_guc(dev_priv);
>>>  }
>

Regards,

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

✗ Fi.CI.BAT: warning for drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission (rev3)
URL   : https://patchwork.freedesktop.org/series/31677/
State : warning

== Summary ==

Series 31677v3 drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission
https://patchwork.freedesktop.org/api/1.0/series/31677/revisions/3/mbox/

Test gem_exec_reloc:
        Subgroup basic-gtt-active:
                dmesg-warn -> PASS       (fi-gdg-551) fdo#102582 +5
        Subgroup basic-write-cpu-active:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-write-gtt-active:
                skip       -> PASS       (fi-gdg-551) fdo#102582
        Subgroup basic-softpin:
                skip       -> PASS       (fi-gdg-551)
Test gem_linear_blits:
        Subgroup basic:
                skip       -> PASS       (fi-gdg-551)
Test gem_render_linear_blits:
        Subgroup basic:
                skip       -> PASS       (fi-gdg-551)
Test gem_render_tiled_blits:
        Subgroup basic:
                skip       -> PASS       (fi-gdg-551)
Test gem_sync:
        Subgroup basic-all:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-each:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-many-each:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-store-each:
                skip       -> PASS       (fi-gdg-551)
Test gem_tiled_blits:
        Subgroup basic:
                skip       -> PASS       (fi-gdg-551)
Test gem_tiled_fence_blits:
        Subgroup basic:
                skip       -> PASS       (fi-gdg-551)
Test gem_wait:
        Subgroup basic-busy-all:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-wait-all:
                skip       -> PASS       (fi-gdg-551)
        Subgroup basic-await-all:
                skip       -> PASS       (fi-gdg-551)
Test kms_busy:
        Subgroup basic-flip-a:
                skip       -> PASS       (fi-gdg-551) fdo#102654 +1
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                skip       -> PASS       (fi-gdg-551) fdo#102618
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:448s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:452s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:370s
fi-bsw-n3050     total:289  pass:242  dwarn:1   dfail:0   fail:0   skip:46  time:533s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:267s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:505s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:499s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:500s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:487s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:547s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:592s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:420s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:252s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:585s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:490s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:432s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:489s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:493s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:570s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:583s
fi-pnv-d510      total:289  pass:223  dwarn:0   dfail:0   fail:0   skip:66  time:540s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:647s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:504s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:453s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:567s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:423s

bcee836068d98bd2aaa5d64124c5994acce6a6c4 drm-tip: 2017y-10m-24d-15h-00m-14s UTC integration manifest
6c1cde528481 drm/i915 : Unifying seq_puts messages for feature support

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6168/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx