[Intel-gfx] [10/11] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"

[Oscar Mateo]

On 03/13/2017 04:49 AM, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 08:28:57AM -0800, Oscar Mateo wrote:
>> A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
>> is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
>> a lot of things called "context" that refer to different things. We can improve legibility
>> a lot by simply renaming a few objects in the GuC code.
>>
>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
>> ---
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a912ec4..e320008 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -35,16 +35,18 @@
>>    * descriptor and a workqueue (all of them inside a single gem object that
>>    * contains all required pages for these elements).
>>    *
>> - * GuC context descriptor:
>> - * During initialization, the driver allocates a static pool of 1024 such
>> - * descriptors, and shares them with the GuC.
>> + * GuC stage descriptor:
>> + * Technically, this is known as a "GuC Context" descriptor in the specs, but
>> + * we use the term "stage" to avoid confusion with all the other things already
>> + * named "context" in the driver. During initialization, the driver allocates
>> + * a static pool of 1024 such descriptors, and shares them with the GuC.
>>    * Currently, there exists a 1:1 mapping between a i915_guc_client and a
>> - * guc_context_desc (via the client's context_index), so effectively only
>> - * one guc_context_desc gets used. This context descriptor lets the GuC know
>> - * about the doorbell, workqueue and process descriptor. Theoretically, it also
>> - * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
>> + * guc_stage_desc (via the client's stage_id), so effectively only one
>> + * gets used. This stage descriptor lets the GuC know about the doorbell,
>> + * workqueue and process descriptor. Theoretically, it also lets the GuC
>> + * know about our HW contexts (context ID, etc...), but we actually
>>    * employ a kind of submission where the GuC uses the LRCA sent via the work
>> - * item instead (the single guc_context_desc associated to execbuf client
>> + * item instead (the single guc_stage_desc associated to execbuf client
>>    * contains information about the default kernel context only, but this is
>>    * essentially unused). This is called a "proxy" submission.
> Some of the above (esp. "this is known as a "GuC Context" descriptor in
> the specs") would be good here. Some might say this is where we expect
> the kerneldoc for structs to be...
>
>>   /*Context descriptor for communicating between uKernel and Driver*/
>> -struct guc_context_desc {
>> +struct guc_stage_desc {
>>   	u32 sched_common_area;
>> -	u32 context_id;
>> +	u32 stage_id;
>>   	u32 pas_id;
>>   	u8 engines_used;
>>   	u64 db_trigger_cpu;
>> @@ -359,7 +359,7 @@ struct guc_policy {
>>   } __packed;
>>
Fair enough, I'll move it. Thanks,
-- Oscar