Re: [Intel-xe] [PATCH] drm/xe : Fix GT looping for standalone media

[Riana Tauro <riana.tauro@intel.com>]

On 6/14/2023 11:24 PM, Matt Roper wrote:
> On Tue, Jun 13, 2023 at 02:39:45PM -0700, Matt Roper wrote:
>> On Tue, Jun 13, 2023 at 03:12:32PM +0530, Riana Tauro wrote:
>>> Currently the id of primary gt is set using gt_count and not
>>> the media gt.
>>>
>>> set gt->info.id of media gt using gt_count
>>>
>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_pci.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index bd1f59b49928..2991cf5365d5 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -590,7 +590,7 @@ static int xe_info_init(struct xe_device *xe,
>>>   		 * up with platforms that support both together.
>>>   		 */
>>>   		drm_WARN_ON(&xe->drm, id != 0);
>>> -		gt->info.id = 1;
>>> +		gt->info.id = xe->info.gt_count++;
>>
>> 1 is always correct today since MTL is the only platform with a
>> standalone media GT, and it's a single-tile platform (so the GTs are
>> 0/1).  This will also be true for any future single-tile platforms with
>> standalone media.
>>
>> If/when we eventually have a platform that's both multi-tile _and_
>> multi-gt, we need to decide how we're going to identify GTs.  Since no
>> such platform exists today, we also don't know for sure whether every
>> tile will have both GTs, or whether only some of the tiles will have
>> multiple GTs.  It's not clear that using gt_count++ will necessarily be
>> the right thing to do; maybe we want to make primary GTs 2*tile and
>> media GTs 2*tile+1 (i.e., intentionally skip IDs if some of the tiles
>> have both and others do not).  We may also want to adjust how we
>> approach GT identification in the uapi in general --- maybe we don't
>> even want to give GTs a global ID anymore and instead want to move
>> toward identifying GT with a (tile_id, gt_id) tuple where gt_id is an
>> intra-tile number (i.e., 0=primary, 1=media).
>>
>> The current GT and tile series aimed to just preserve the existing uapi
>> without changes, and using an ID of 1 for media (with all the FIXME
>> comments in the code) does that for now.  But sometime soon we do need
>> to make a formal decision on exactly how we want to work with tiles and
>> GTs in the uapi, going forward and that needs to be something that we
>> loop in our userspace partners on; we can't just make the decision
>> ourselves on the kernel side.
> 
> So looking at this patch again, the important part isn't actually the ID
> of the GT (that's already being set correctly); what's important is that
> we need to bump the GT count again when initializing the media GT,
> otherwise gt_count will stay "1" forever, even when it should be 2.  So
> with an updated commit message that focuses on the gt_count rather than
> the ID,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
Thanks for the review Matt

Sorry for not being clear in the commit message. for_each_gt remains 
stuck at 1 and the media gt was not getting initialized causing an error.

Thanks
Riana
> I can make that tweak to the commit message while applying; no need to
> re-send.
> 
> 
> Matt
> 
>>
>>
>> Matt
>>
>>>   	}
>>>   
>>>   	return 0;
>>> -- 
>>> 2.40.0
>>>
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>