[Intel-gfx] [PATCH 04/10] drm/i915/guc: Add a second client, to be used for preemption

[Daniele Ceraolo Spurio]

On 05/10/17 09:00, Michał Winiarski wrote:
> On Thu, Oct 05, 2017 at 03:22:59PM +0000, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 05/10/17 02:13, Michał Winiarski wrote:
>>> From: Dave Gordon <david.s.gordon at intel.com>
>>>
>>> This second client is created with priority KMD_HIGH, and marked
>>> as preemptive. This will allow us to request preemption using GuC actions.
>>>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Jeff Mcgee <jeff.mcgee at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>>> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_debugfs.c        |  4 ++++
>>>    drivers/gpu/drm/i915/i915_guc_submission.c | 32 ++++++++++++++++++++++++++----
>>>    drivers/gpu/drm/i915/intel_uc.h            |  1 +
>>>    3 files changed, 33 insertions(+), 4 deletions(-)
> 
> [SNIP]
> 
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>>> index 10e8f0ed02e4..c6c6f8513bbf 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.h
>>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>>> @@ -107,6 +107,7 @@ struct intel_guc {
>>>    	struct ida stage_ids;
>>>    	struct i915_guc_client *execbuf_client;
>>> +	struct i915_guc_client *preempt_client;
>>>    	DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
>>>    	uint32_t db_cacheline;		/* Cyclic counter mod pagesize	*/
>>>
>>
>>
>> I think you also need to update guc_init_doorbell_hw() to handle the new
>> client. The comment in there says:
>>
>> /* Now for every client (and not only execbuf_client) make sure their
>>   * doorbells are known by the GuC */
> 
> But I don't want the doorbell for preempt_client...
> I'm not submitting anything 'directly' using this client (just indirectly -
> through preempt action).
> 
> Are you sure we need the doorbell?
> Last time I tried to refactor GuC submission to use doorbell per engine, I got
> the info that I should *avoid* allocating multiple doorbells.
> 
> -Michał
> 

Don't you still get a doorbell from guc_client_alloc()? or am I missing 
something?
One of the issues that guc_init_doorbell_hw() tries to solve is that 
after a GuC reset the doorbell HW is not cleaned up, so you can 
potentially have an active doorbell that GuC doesn't know about, which 
could lead to a failure if we try to release that doorbell later on. By 
doing the H2G we are sure that HW and GuC FW are in sync and I think 
this should apply to the doorbell in the preempt client as well.

The problem with having many doorbells is that they add a slight delay 
when waking the power well (not sure about the exact details) and AFAIK 
this happens even if you don't ring them. For this reason it is true 
that we want to keep the number of doorbells limited, but having 2 in 
total shouldn't be an issue. However, if we really don't need the 
doorbell then we should probably modify guc_client_alloc() to skip the 
doorbell allocation for the preempt client, but that could lead to a 
bigger rework to remove the assumption that each client has a doorbell.

Daniele

>>
>> Daniele