Re: [PATCH 1/2] drm/xe/display: Move dpt allocation to helper

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Den 2025-01-14 kl. 20:22, skrev Cavitt, Jonathan:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Juha-Pekka Heikkila
> Sent: Tuesday, January 14, 2025 10:04 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Subject: [PATCH 1/2] drm/xe/display: Move dpt allocation to helper
>>
>> Simplify __xe_pin_fb_vma_dpt() by moving dpt allocation into helper.
>> This also fixes bug where dpt could have been allocated from system
>> memory when on dgfx.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   drivers/gpu/drm/xe/display/xe_fb_pin.c | 67 +++++++++++++++++---------
>>   1 file changed, 43 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> index 9fa51b84737c..c28885316986 100644
>> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> @@ -77,6 +77,47 @@ write_dpt_remapped(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs,
>>   	*dpt_ofs = ALIGN(*dpt_ofs, 4096);
>>   }
>>   
>> +static struct xe_bo *xe_alloc_dpt_bo(struct xe_device *xe,
>> +				     struct xe_tile *tile0, u64 size,
>> +				     u64 physical_alignment)
>> +{
>> +	struct xe_bo *dpt;
>> +
>> +	/*
>> +	 * If DGFX: try VRAM0 only
>> +	 */
>> +	if (IS_DGFX(xe)) {
>> +		dpt = xe_bo_create_pin_map_at_aligned(xe, tile0, NULL,
>> +						      size, ~0ull,
>> +						      ttm_bo_type_kernel,
>> +						      XE_BO_FLAG_VRAM0 |
>> +						      XE_BO_FLAG_GGTT |
>> +						      XE_BO_FLAG_PAGETABLE,
>> +						      physical_alignment);
>> +	} else {
>> +		/*
>> +		 * For IGFX: first try STOLEN. on fail try SYSTEM.
>> +		 */
>> +		dpt = xe_bo_create_pin_map_at_aligned(xe, tile0, NULL,
>> +						      size, ~0ull,
>> +						      ttm_bo_type_kernel,
>> +						      XE_BO_FLAG_STOLEN |
>> +						      XE_BO_FLAG_GGTT |
>> +						      XE_BO_FLAG_PAGETABLE,
>> +						      physical_alignment);
>> +		if (IS_ERR(dpt)) {
>> +			dpt = xe_bo_create_pin_map_at_aligned(xe, tile0, NULL,
>> +							      size, ~0ull,
>> +							      ttm_bo_type_kernel,
>> +							      XE_BO_FLAG_SYSTEM |
>> +							      XE_BO_FLAG_GGTT |
>> +							      XE_BO_FLAG_PAGETABLE,
>> +							      physical_alignment);
>> +		}
>> +	}
>> +	return dpt;
> 
> We might be able to collapse some of this logic by storing the flags separately:
> 
> """
> static struct xe_bo *xe_alloc_dpt_bo(struct xe_device *xe,
> 				     struct xe_tile *tile0, u64 size,
> 				     u64 physical_alignment)
> {
> 	struct xe_bo *dpt;
> 	u32 base_flags = XE_BO_FLAG_GGTT | XE_BO_FLAG_PAGETABLE;
> 	u32 flags = base_flags;
> 
> 	/*
> 	 * If DGFX: try VRAM0.
> 	 * If IGFX: try STOLEN.
> 	 */
> 	flags |= IS_DGFX(xe) ? XE_BO_FLAG_VRAM0 : XE_BO_FLAG_STOLEN;
> 
> 	dpt = xe_bo_create_pin_map_at_aligned(xe, tile0, NULL, size,
> 					      ~0ull, ttm_bo_type_kernel,
> 					      flags, physical_alignment);
> 
> 	/*
> 	 * For IGFX, we first try STOLEN, and on a failure we try SYSTEM.
> 	 * DGFX should only attempt VRAM0
> 	 */
> 	if (IS_DGFX(xe) && IS_ERR(dpt))
> 		dpt = xe_bo_create_pin_map_at_aligned(xe, tile0, NULL,
> 						      size, ~0ull,
> 						      ttm_bo_type_kernel,
> 						      base_flags |
> 						      XE_BO_FLAG_SYSTEM,
> 						      physical_alignment);
> 	return dpt;
> }
> """
> This isn't a particularly necessary compression, but it might be worth considering.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>Except that fails on both integrated and discrete, due to IS_DGFX() used 
wrongly here. ;-)

Every change, no matter how small, has the opportunity to break things.

Regardless, for both patches:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Cheers,
~Maarten