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

[Cavitt, Jonathan]

-----Original Message-----
From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> 
Sent: Wednesday, January 15, 2025 2:19 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>; intel-xe at lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/xe/display: Move dpt allocation to helper
> Den 2025-01-14 kl. 20:22, skrev Cavitt, Jonathan:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Juha-Pekka Heikkila
> > Sent: Tuesday, January 14, 2025 10:04 AM
> > To: intel-xe at lists.freedesktop.org
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at 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 at 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 at 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.

Interesting.  For future reference, may I ask how IS_DGFX is being used incorrectly in the above example?
-Jonathan Cavitt

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