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

[Cavitt, Jonathan]

-----Original Message-----
From: Cavitt, Jonathan <jonathan.cavitt at intel.com> 
Sent: Wednesday, January 15, 2025 7:10 AM
To: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>; Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>; intel-xe at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: RE: [PATCH 1/2] drm/xe/display: Move dpt allocation to helper
> 
> -----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?

Ah, wait.  Never mind, I see it.  I forgot to negate IS_DGFX in the error case.  It should be:
"""
	if (!IS_DGFX(xe) && IS_ERR(dpt))
"""
-Jonathan Cavitt

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