Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Michel Thierry <michel.thierry@intel.com>]

On 12/10/17 13:35, Michel Thierry wrote:
> On 09/10/17 15:35, Michel Thierry wrote:
>> On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote:
>>>
>>>
>>> On 09/10/17 07:52, Michał Winiarski wrote:
>>>> We were using first page of kernel context render state for sharing 
>>>> data
>>>> with GuC. While it's justified by the fact that those pages are not 
>>>> used
>>>> (note, GuC still enforces this layout and refuses to work if we remove
>>>> the extra page in front), it's also confusing (why are we using this
>>>> particular page?). Let's allocate a separate object instead.
>>>>
>>>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Jeff McGee <jeff.mcgee@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>>
>>> +Michel (engine and watchdog reset with GuC use the shared page)
>>>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 36 
>>>> +++++++++++++++++++++++++++++-
>>>>   drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
>>>>   drivers/gpu/drm/i915/intel_guc.h           |  2 ++
>>>>   3 files changed, 39 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 8983d53af229..30f026566001 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct 
>>>> intel_guc *guc,
>>>>       memset(desc, 0, sizeof(*desc));
>>>>   }
>>>> +static int guc_shared_data_create(struct intel_guc *guc)
>>>> +{
>>>> +    struct i915_vma *vma;
>>>> +    void *vaddr;
>>>> +
>>>> +    vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>>>> +    if (IS_ERR(vma))
>>>> +        return PTR_ERR(vma);
>>>> +
>>>> +    vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
>>>> +    if (IS_ERR(vaddr)) {
>>>> +        i915_vma_unpin_and_release(&vma);
>>>> +        return PTR_ERR(vaddr);
>>>> +    }
>>>> +
>>>> +    guc->shared_data = vma;
>>>> +    guc->shared_data_vaddr = vaddr;
>>
>> Hi,
>>
>> Allocating the shared_data until this point (i915_guc_submission_init) 
>> will be too late for GuC's watchdog.
>>
>> GuC watchdog happens without i915 knowledge, so we have to pass this 
>> shared_data_offset during guc_params_init (in params[9] for the 
>> curious) instead of a h2g command; and the GuC parameters block has 
>> this note: "These parameters are read by the firmware on startup and 
>> cannot be changed thereafter".
>>
>> Michał, if you plan to send another version of this, could you move it 
>> to guc_params_init? It isn't a big issue, I can just move it when we 
>> have an open source user and can upstream GuC watchdog.
>>
>> Thanks,
>>
>> -Michel
>>
> 
> Ignore my previous reply, this is already being allocated before 
> guc_params_init as it is.
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> 

I spoke too soon (and sorry for all the spam), the kernel_context is now 
redundant code and should be removed from the suspend & resume functions:

diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c
index 5cd9bc53e021..47c74ef0bd23 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -176,7 +176,6 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 
rsa_offset)
  int intel_guc_suspend(struct drm_i915_private *dev_priv)
  {
  	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_gem_context *ctx;
  	u32 data[3];

  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
@@ -184,8 +183,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)

  	gen9_disable_guc_interrupts(dev_priv);

-	ctx = dev_priv->kernel_context;
-
  	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
  	/* any value greater than GUC_POWER_D0 */
  	data[1] = GUC_POWER_D1;
@@ -225,7 +222,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
  int intel_guc_resume(struct drm_i915_private *dev_priv)
  {
  	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_gem_context *ctx;
  	u32 data[3];

  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
@@ -234,8 +230,6 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
  	if (i915_modparams.guc_log_level >= 0)
  		gen9_enable_guc_interrupts(dev_priv);

-	ctx = dev_priv->kernel_context;
-
  	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
  	data[1] = GUC_POWER_D0;
  	data[2] = guc_ggtt_offset(guc->shared_data);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx