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

[Michel Thierry]

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 at intel.com>
>>> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Jeff McGee <jeff.mcgee at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at 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 at intel.com>

>>
>> I've noticed that this is now the 3rd place where we allocate and 
>> immediately pin a vma for guc (the other 2 being 
>> guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 
>> operations to a more common helper (and do the same for the cleanup), 
>> but it's probably better to do it after all the ongoing GuC re-org has 
>> settled down. In the meantime, this patch is:
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>
>> Daniele