[Intel-xe] [PATCH 1/1] drm/xe: Fix stolen memory size calculation

[Ruhl, Michael J]

>-----Original Message-----
>From: Auld, Matthew <matthew.auld at intel.com>
>Sent: Monday, June 12, 2023 11:56 AM
>To: Iddamsetty, Aravind <aravind.iddamsetty at intel.com>; Ruhl, Michael J
><michael.j.ruhl at intel.com>; intel-xe at lists.freedesktop.org
>Cc: Roper, Matthew D <matthew.d.roper at intel.com>
>Subject: Re: [Intel-xe] [PATCH 1/1] drm/xe: Fix stolen memory size calculation
>
>On 12/06/2023 16:04, Iddamsetty, Aravind wrote:
>>
>>
>> On 12-06-2023 19:59, Ruhl, Michael J wrote:
>>>> -----Original Message-----
>>>> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
>>>> Aravind Iddamsetty
>>>> Sent: Friday, June 9, 2023 2:53 PM
>>>> To: intel-xe at lists.freedesktop.org
>>>> Cc: Roper, Matthew D <matthew.d.roper at intel.com>
>>>> Subject: [Intel-xe] [PATCH 1/1] drm/xe: Fix stolen memory size
>calculation
>>>>
>>>> The stolen memory base should be derived from GSMBASE or
>>>> FLAT_CCS_BASE_ADDR when flat ccs is applicable.
>>>>
>>>> Fixes: acf067c2f5a5 ("drm/xe: Implement stolen memory.")
>>>>
>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>>>> b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>>>> index 21ecc734f10a..29ce69bf8200 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>>>> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>>>> @@ -54,7 +54,6 @@ bool xe_ttm_stolen_cpu_access_needs_ggtt(struct
>>>> xe_device *xe)
>>>> static s64 detect_bar2_dgfx(struct xe_device *xe, struct
>xe_ttm_stolen_mgr
>>>> *mgr)
>>>> {
>>>> 	struct xe_tile *tile = xe_device_get_root_tile(xe);
>>>> -	struct xe_gt *mmio = xe_root_mmio_gt(xe);
>>>> 	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>>> 	u64 stolen_size;
>>>> 	u64 tile_offset;
>>>> @@ -66,12 +65,12 @@ static s64 detect_bar2_dgfx(struct xe_device *xe,
>>>> struct xe_ttm_stolen_mgr *mgr)
>>>> 		return 0;
>>>> 	}
>>>>
>>>> -	/* Use DSM base address instead for stolen memory */
>>>> -	mgr->stolen_base = (xe_mmio_read64(mmio, DSMBASE) &
>>BDSM_MASK) - tile_offset;
>>>> +	/* stolen base should be at end of usable vram */
>>>> +	mgr->stolen_base = vram_size;
>>>
>>> Hi Arvind,
>>>
>>> The stolen memory in i915 is not calculated like this.  It uses the DSMBASE
>for the offset like the
>>> current code.
>>>
>>> The lmem size includes the CSS sizing, but that is for the LMEM memory
>region.
>>>
>>> This is calculating and creating the "stolen" region for use in kernel space.
>If the offset includes the CSS space,
>>> won't the DSM usage be in the wrong place?
>>
>> I didn't realize until you mentioned here we wanted to carve out usable
>> stolen in which case what I did is wrong, makes sense.
>>
>> but why is that we are doing only on root tile even remote tile has
>> stolen area right?
>
>I think the main reason is because there is no user in the driver for it
>outside of display stuff, and display only cares about root tile atm. If
>we can actively utilise it outside of display (like some per tile
>resource needed by the driver) then maybe it makes sense to probe it for
>all tiles, assuming it's still present on such platforms.

I did move this from the xe device to the gt (tile?) device with multi-tile in mind, but
because of the above comments, I did not update to do the calculation for all tiles.

Mike


>>
>> Thanks,
>> Aravind.
>>>
>>>> 	if (drm_WARN_ON(&xe->drm, tile_size < mgr->stolen_base))
>>>> 		return 0;
>>>>
>>>> -	stolen_size = tile_size - mgr->stolen_base;
>>>> +	stolen_size = tile_size - vram_size;
>>>
>>> The tile size was picked to work with devices that do not have tile
>information (pci_resource _len used).  vram_size
>>> can be wrong in those instances.
>>>
>>> My original patch set used the vram_size, but that was not accurate for this
>usage.
>>>
>>> Are you seeing an issue with the current code?
>>>
>>> Thanks
>>>
>>> Mike
>>>
>>>> 	/* Verify usage fits in the actual resource available */
>>>> 	if (mgr->stolen_base + stolen_size <= pci_resource_len(pdev,
>>GEN12_LMEM_BAR))
>>>> --
>>>> 2.25.1
>>>