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

[Michel Thierry]

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

> 
> 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