[Intel-xe] [PATCH 1/4] drm/xe: Rework size helper to be a little more correct

[Ruhl, Michael J]

>-----Original Message-----
>From: Auld, Matthew <matthew.auld at intel.com>
>Sent: Wednesday, May 3, 2023 11:42 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
>xe at lists.freedesktop.org
>Cc: Brost, Matthew <matthew.brost at intel.com>; Kershner, David
><david.kershner at intel.com>; Ghimiray, Himal Prasad
><himal.prasad.ghimiray at intel.com>; Upadhyay, Tejas
><tejas.upadhyay at intel.com>
>Subject: Re: [PATCH 1/4] drm/xe: Rework size helper to be a little more
>correct

<snip>

>>>>>>     	/* Use DSM base address instead for stolen memory */
>>>>>> -	mgr->stolen_base = xe_mmio_read64(gt, DSMBASE.reg) &
>>>>> BDSM_MASK;
>>>>>> -	if (drm_WARN_ON(&xe->drm, vram_size < mgr-
>>stolen_base))
>>>>>> +	mgr->stolen_base = (xe_mmio_read64(gt, DSMBASE.reg) &
>>>>> BDSM_MASK) - tile_offset;
>>>>>> +	if (drm_WARN_ON(&gt->xe->drm, tile_size < mgr-
>>stolen_base))
>>>>>>     		return 0;
>>>>>>
>>>>>> -	stolen_size = vram_size - mgr->stolen_base;
>>>>>> -	if (mgr->stolen_base + stolen_size <= pci_resource_len(pdev,
>2))
>>>>>> -		mgr->io_base = pci_resource_start(pdev, 2) + mgr-
>>>>>> stolen_base;
>>>>>> +	stolen_size = tile_size - mgr->stolen_base;
>>>>>> +
>>>>>> +	if (mgr->stolen_base + stolen_size <= tile_size)
>>>>>
>>>>> I think needs to be the pci_len() here.
>>>>
>>>> For a PVC that would be 128GB for length.  That will encompass both tiles,
>>> and I think that the
>>>> this will not check correctly.
>>>>
>>>> tile_size == pci_len() for DG1...
>>>>
>>>> Is that your concern?
>>>>
>>>> I think that he "stolen" area has to be constrained to the tile size, not the
>>> complete area.
>>>> Am I missing something here?
>>>
>>> Just that we want mgr->io_base here to be zero on small-bar, which then
>>> signals that stolen is not directly CPU accessible. Also we only care
>>> about the root tile here.
>>
>> Ahh, ok.
>>
>> Umm, so
>>
>> size = min(gt.vram.io_size, tile_size)
>
>I think gt->vram.io_size is the per-GT io_size clamped to the usable
>VRAM size, so is always < tile_size. We want the entire tile based
>io_size which we don't seem to track, but since we only care about root
>tile here we can just use pci_len() I think (as per the old behaviour).

Ahh, ok, so we are verifying that the size will fit in the resource, not that the
size fits the tile correctly.

Will go back to the pci_resource_len() usage.

>>
>> ?
>>
>> The stolen area is defined per GT, so I updated for the possibility of others...
>Should we
>> restrict it for GT0 only?
>
>Yeah, existing code assumes GT0 it seems. IIRC stolen is mostly used for
>display stuff and multi-tile tends to lack display. Not sure if we ditch
>DSM altogether for PVC like platforms?

It is in the memory map for PVC, and the current code base walks it...

Since the memory map has it for each tile, I think keeping the GT specific
code path is reasonable...but will go back to the xe only if you think that is
best.

Regards,

Mike


>>
>> M
>>
>>>>
>>>> Thanks
>>>>
>>>> Mike
>>>>
>>>>>> +		mgr->io_base = gt->mem.vram.io_start + mgr-
>>stolen_base;
>>>>>>
>>>>>>     	/*
>>>>>>     	 * There may be few KB of platform dependent reserved
>memory at
>>>>> the end
>>>>>> @@ -139,7 +139,7 @@ void xe_ttm_stolen_mgr_init(struct xe_device
>>> *xe)
>>>>>>     	int err;
>>>>>>
>>>>>>     	if (IS_DGFX(xe))
>>>>>> -		stolen_size = detect_bar2_dgfx(xe, mgr);
>>>>>> +		stolen_size = detect_bar2_dgfx(to_gt(xe), mgr);
>>>>>>     	else if (GRAPHICS_VERx100(xe) >= 1270)
>>>>>>     		stolen_size = detect_bar2_integrated(xe, mgr);
>>>>>>     	else