[PATCH 1/2] drm/xe/display: Move dpt allocation to helper
Juha-Pekka Heikkilä
juhapekka.heikkila at gmail.com
Thu Jan 16 15:33:40 UTC 2025
Thanks for the reviews,
On Wed, Jan 15, 2025 at 5:26 PM Cavitt, Jonathan
<jonathan.cavitt at intel.com> wrote:
>
> -----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:
I was initially thinking something similar but didn't come up with
anything that I did like hence I decided to stick with the looks of
original code. Your version look better than those I had, maybe first
put these patches in as is since it fixes a bug and later come back
for this.
/Juha-Pekka
More information about the Intel-xe
mailing list