Re: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot

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

On 11/8/2024 15:34, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Friday, November 8, 2024 3:10 PM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>; Dong, Zhanjun <zhanjun.dong@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
>> On Fri, Nov 08, 2024 at 03:05:34PM -0700, Cavitt, Jonathan wrote:
>>> -----Original Message-----
>>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Matthew Brost
>>> Sent: Friday, November 8, 2024 9:43 AM
>>> To: intel-xe@lists.freedesktop.org
>>> Cc: Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>; Dong, Zhanjun <zhanjun.dong@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>> Subject: [PATCH 2/7] drm/xe: Add ring address to LRC snapshot
>>>> The ring is currently in LRC BO but this may change going forward.
>>>> Include the ring address in the snapshot protecting again any future
>>>> changes.
>>>>
>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> LGTM, though the terminology we're using to describe the various ggtt addresses
>>> as "descriptors" is a bit confusing, even if it's consistent.  I wonder where that
>>> terminology came from?
>>> This is just a rhetorical question.  I'm not suggesting it be changed.
>>>
>> LRC descriptors is name copied over from i915 and may even be in the bspec.
>>
>> But yea, indirect_state_desc and ring_desc are bad names. Will change to
>> 'ring_addr' here.
> IMO it would probably be better to leave it as "ring_desc" for now as it's
> consistent with surrounding struct members.  We can do a pass of the full
> XE kernel for inaccurate uses of the "desc" qualifier in the near future and
> fix the naming scheme here as a part of that fixup.
I strongly disagree. Just because A is broken doesn't mean we should add 
a broken B and C! It makes no sense to add a name we know is bad just so 
that we can change it later. Anything new should be done properly from 
the start.

John.

>
> My RB still stands either way.
> -Jonathan Cavitt
>
>> Matt
>>
>>> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>> -Jonathan Cavitt
>>>
>>>> ---
>>>>   drivers/gpu/drm/xe/xe_lrc.c | 3 +++
>>>>   drivers/gpu/drm/xe/xe_lrc.h | 1 +
>>>>   2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>>>> index e219657535cf..afb0f4f44748 100644
>>>> --- a/drivers/gpu/drm/xe/xe_lrc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_lrc.c
>>>> @@ -1636,6 +1636,7 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
>>>>   		xe_vm_get(lrc->bo->vm);
>>>>   
>>>>   	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
>>>> +	snapshot->ring_desc = __xe_lrc_ring_ggtt_addr(lrc);
>>>>   	snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
>>>>   	snapshot->head = xe_lrc_ring_head(lrc);
>>>>   	snapshot->tail.internal = lrc->ring.tail;
>>>> @@ -1693,6 +1694,8 @@ void xe_lrc_snapshot_print(struct xe_lrc_snapshot *snapshot, struct drm_printer
>>>>   		return;
>>>>   
>>>>   	drm_printf(p, "\tHW Context Desc: 0x%08x\n", snapshot->context_desc);
>>>> +	drm_printf(p, "\tHW Ring: 0x%08x\n",
>>>> +		   snapshot->ring_desc);
>>>>   	drm_printf(p, "\tHW Indirect Ring State: 0x%08x\n",
>>>>   		   snapshot->indirect_context_desc);
>>>>   	drm_printf(p, "\tLRC Head: (memory) %u\n", snapshot->head);
>>>> diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
>>>> index 9d64cedc4d14..a2058a501353 100644
>>>> --- a/drivers/gpu/drm/xe/xe_lrc.h
>>>> +++ b/drivers/gpu/drm/xe/xe_lrc.h
>>>> @@ -25,6 +25,7 @@ struct xe_lrc_snapshot {
>>>>   	unsigned long lrc_size, lrc_offset;
>>>>   
>>>>   	u32 context_desc;
>>>> +	u32 ring_desc;
>>>>   	u32 indirect_context_desc;
>>>>   	u32 head;
>>>>   	struct {
>>>> -- 
>>>> 2.34.1
>>>>
>>>>