Re: [PATCH] drm/xe/xe_lrc: Rename desc to addr

[John Harrison <john.c.harrison@intel.com>]

On 11/12/2024 11:57, Matt Roper wrote:
> On Tue, Nov 12, 2024 at 06:55:15PM +0000, Jonathan Cavitt wrote:
>> The qualifier "desc" as used in the xe_lrc_snapshot struct is a legacy
>> term used by i915 that has been incorrectly applied to some of the
> No, "context descriptor" is a formal hardware term that's very much
> still in use.  The context descriptor is the 64-bit value that gets
> programmed into execlist submission port to ask the hardware to run a
> context.  The context descriptor contains the LRCA inside of it (among
> other things).  See for example bspec 60419, 60156, etc.
>
> So the real question is what is the capture code here trying to capture
> and dump?  The full 64-bit context descriptor or just the LRCA?  If the
> goal was to capture the full descriptor, then we should probably just
> make it do that.
>
>
> Matt
>
>> attributes.  Said attributes that use the "desc" qualifier, namely
>> "context_desc" and "indirect_context_desc", would more accurately be
>> referred to as addresses, so replace the incorrect "desc" qualifier with
>> "addr" in these cases.
>>
>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>> CC: John Harrison <john.c.harrison@intel.com>
>> CC: Matthew Brost <matthew.brost@intel.com>
>> CC: Zhanjun Dong <zhanjun.dong@intel.com>
>> CC: Alan Previn <alan.previn.teres.alexis@intel.com>
>> CC: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_capture.c | 2 +-
>>   drivers/gpu/drm/xe/xe_lrc.c         | 8 ++++----
>>   drivers/gpu/drm/xe/xe_lrc.h         | 4 ++--
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
>> index cc72446a5de1..b95c21a18c31 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
>> @@ -1632,7 +1632,7 @@ xe_engine_manual_capture(struct xe_hw_engine *hwe, struct xe_hw_engine_snapshot
>>   		if (ge) {
>>   			guc_id = ge->guc.id;
>>   			if (ge->lrc[0])
>> -				lrca = ge->lrc[0]->context_desc;
>> +				lrca = ge->lrc[0]->context_addr;
>>   		}
>>   	}
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>> index 4b65da77c6e0..09f0e66952e7 100644
>> --- a/drivers/gpu/drm/xe/xe_lrc.c
>> +++ b/drivers/gpu/drm/xe/xe_lrc.c
>> @@ -1635,8 +1635,8 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
>>   	if (lrc->bo->vm)
>>   		xe_vm_get(lrc->bo->vm);
>>   
>> -	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
>> -	snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
>> +	snapshot->context_addr = xe_lrc_ggtt_addr(lrc);
>> +	snapshot->indirect_context_addr = xe_lrc_indirect_ring_ggtt_addr(lrc);
>>   	snapshot->head = xe_lrc_ring_head(lrc);
>>   	snapshot->tail.internal = lrc->ring.tail;
>>   	snapshot->tail.memory = xe_lrc_ring_tail(lrc);
>> @@ -1692,9 +1692,9 @@ void xe_lrc_snapshot_print(struct xe_lrc_snapshot *snapshot, struct drm_printer
>>   	if (!snapshot)
>>   		return;
>>   
>> -	drm_printf(p, "\tHW Context Desc: 0x%08x\n", snapshot->context_desc);
>> +	drm_printf(p, "\tHW Context Address: 0x%08x\n", snapshot->context_addr);
>>   	drm_printf(p, "\tHW Indirect Ring State: 0x%08x\n",
>> -		   snapshot->indirect_context_desc);
>> +		   snapshot->indirect_context_addr);
This is neither a context descriptor nor a context address (indirect or 
otherwise). As per the print text, it is the address of the indirect 
ring state object. One could call that a context in the general sense 
that 'context', 'state', 'info', 'storage', etc. can all be used to 
describe the saved state of an entity. But generally, we use context 
specifically for the entire 'thing' that needs to be saved and restored 
to switch the hardware from one user to another. The indirect ring state 
is just a small sub-part of that.

John.

>>   	drm_printf(p, "\tLRC Head: (memory) %u\n", snapshot->head);
>>   	drm_printf(p, "\tLRC Tail: (internal) %u, (memory) %u\n",
>>   		   snapshot->tail.internal, snapshot->tail.memory);
>> diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
>> index 40d8f6906d3e..f6bf81009294 100644
>> --- a/drivers/gpu/drm/xe/xe_lrc.h
>> +++ b/drivers/gpu/drm/xe/xe_lrc.h
>> @@ -24,8 +24,8 @@ struct xe_lrc_snapshot {
>>   	void *lrc_snapshot;
>>   	unsigned long lrc_size, lrc_offset;
>>   
>> -	u32 context_desc;
>> -	u32 indirect_context_desc;
>> +	u32 context_addr;
>> +	u32 indirect_context_addr;
>>   	u32 head;
>>   	struct {
>>   		u32 internal;
>> -- 
>> 2.43.0
>>