[PATCH] drm/xe/fbdev: Limit the usage of stolen for LNL+

Sun Jul 14 19:49:55 UTC 2024

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Thursday, July 11, 2024 10:37 PM
> To: Roper, Matthew D <matthew.d.roper at intel.com>
> Cc: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
> intel-xe at lists.freedesktop.org; ville.syrjala at linux.intel.com; Ceraolo Spurio,
> Daniele <daniele.ceraolospurio at intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar at intel.com>
> Subject: Re: [PATCH] drm/xe/fbdev: Limit the usage of stolen for LNL+
> 
> On Thu, Jul 11, 2024 at 09:37:41AM GMT, Matt Roper wrote:
> >On Thu, Jul 11, 2024 at 10:43:39AM +0530, Uma Shankar wrote:
> >> As per recommendation in the workarounds:
> >> WA_14021987551, Wa_16023588340:
> >
> >The first one here isn't a valid workaround lineage number, just a
> >per-platform ticket number.  I think you meant Wa_22019338487, which is
> >the lineage number that can be used to track the applicability of the
> >workaround across all potentially relevant platform(s).
> >
> >>
> >> There is an issue with accessing Stolen memory pages due a hardware
> >> limitation. Limit the usage of stolen memory for fbdev for LNL+.
> >> Don't use BIOS FB from stolen on LNL+ and
> >
> >From a quick skim of these workarounds I don't see a clear indication
> >that we need to avoid using stolen FB's?  I thought these workarounds
> >were already being implemented separately (and aside from disabling
> >FBC, most of the necessary changes are on the GT side to adjust
> >frequencies and change caching behavior of LMEMBAR accesses).  I.e.,
> 
> one part of Wa_22019338487 is what is implemented in
> ggtt_update_access_counter(), because ggtt is in stolen. This can be done for
> things under kernel-only control. For other things like the fb we need to avoid
> them.

Yeah right.

> 
> Also, while thinking about that... Did we check if we also need
> something for DPT? AFAICS for LNL we will end up in
> 
>                  dpt = xe_bo_create_pin_map(xe, tile0, NULL, dpt_size,
>                                             ttm_bo_type_kernel,
>                                             XE_BO_FLAG_STOLEN |
>                                             XE_BO_FLAG_GGTT |
>                                             XE_BO_FLAG_PAGETABLE);
> 
> 
> ... and I don't see anything fencing the writes like we have in ggtt.

For DPT as per some discussions with windows team, it seems this should not harm
as CPU accesses will be limited. Will move this out if we encounter any corner
case. So far this seems to be ok as per the CI and local testing.

Regards,
Uma Shankar

> 
> >
> > - Wa_16023588340: https://patchwork.freedesktop.org/series/135699/
> > - Wa_22019338487: https://patchwork.freedesktop.org/series/135460/
> >
> >One other nitpick:  we've been trying to avoid using "+" as shorthand
> >for "and beyond" lately since some of our IP names contain literal +'s
> >in their name (e.g., Xe_LPD+, Xe_LPM+, etc.).  We don't want confusion
> >about whether "LNL+" refers to "LNL and beyond" (as you mean here) or
> >some other platform that's distinct from LNL.
> >
> >But in general we usually don't treat workarounds as "forever" logic
> >unless they get written into the spec as new "official" programming.  It
> >looks like these workarounds apply to LNL/BMG today, but we shouldn't
> >assume in advance that they'll automatically apply to platforms n+1,
> >n+2, etc. as well.
> >
> >If we're making a concious decision to apply workaround programming to
> >more platforms than it's technically needed on (e.g., in cases where a
> >workaround doesn't have any negative impact, but applying it
> >unconditionally simplifies the driver logic), that should be called out
> >specifically in the commit message and comments to make it clear it
> >isn't a mistake.  But I don't think that's the case here, since
> >otherwise we wouldn't be bothering with the DISPLAY_VER < 20 condition
> >either.
> 
> this is basically Wa_22019338487 and not exactly related with the
> *display* version, hence my suggestion in the other reply to use XE_WA()
> and tie it either to the platform or GRAPHICS_VERSION(2004)
> 
> Lucas De Marchi
> 
> >
> >
> >Matt
> >
> >> assign the same from system memory.
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/display/intel_fbdev_fb.c   | 10 +++++++++-
> >>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 10 ++++++++++
> >>  2 files changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >> index 816ad13821a8..8fda8745ce0a 100644
> >> --- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >> +++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
> >> @@ -37,7 +37,14 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct
> drm_fb_helper *helper,
> >>  	size = PAGE_ALIGN(size);
> >>  	obj = ERR_PTR(-ENODEV);
> >>
> >> -	if (!IS_DGFX(xe)) {
> >> +	/*
> >> +	 * WA_14021987551, Wa_16023588340:
> >> +	 * There is an issue with accessing Stolen memory pages
> >> +	 * due a hardware limitation. Limit the usage of stolen
> >> +	 * memory for fbdev for LNL+. Don't use BIOS FB from
> >> +	 * stolen on LNL+ and assign the same from system memory
> >> +	 */
> >> +	if (!IS_DGFX(xe) && (DISPLAY_VER(xe) < 20)) {
> >>  		obj = xe_bo_create_pin_map(xe, xe_device_get_root_tile(xe),
> >>  					   NULL, size,
> >>  					   ttm_bo_type_kernel,
> XE_BO_FLAG_SCANOUT |
> >> @@ -48,6 +55,7 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct
> drm_fb_helper *helper,
> >>  		else
> >>  			drm_info(&xe->drm, "Allocated fbdev into stolen failed:
> %li\n", PTR_ERR(obj));
> >>  	}
> >> +
> >>  	if (IS_ERR(obj)) {
> >>  		obj = xe_bo_create_pin_map(xe, xe_device_get_root_tile(xe),
> NULL, size,
> >>  					   ttm_bo_type_kernel,
> XE_BO_FLAG_SCANOUT |
> >> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> >> index 5eccd6abb3ef..80dd6b64c921 100644
> >> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> >> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> >> @@ -104,6 +104,16 @@ initial_plane_bo(struct xe_device *xe,
> >>  		phys_base = base;
> >>  		flags |= XE_BO_FLAG_STOLEN;
> >>
> >> +		/*
> >> +		 * WA_14021987551, Wa_16023588340:
> >> +		 * There is an issue with accessing Stolen memory pages
> >> +		 * due a hardware limitation. Limit the usage of stolen
> >> +		 * memory for fbdev for LNL+. Don't use BIOS FB from
> >> +		 * stolen on LNL+ and assign the same from system memory
> >> +		 */
> >> +		if (DISPLAY_VER(xe) >= 20)
> >> +			return NULL;
> >> +
> >>  		/*
> >>  		 * If the FB is too big, just don't use it since fbdev is not very
> >>  		 * important and we should probably use that space with FBC or
> other
> >> --
> >> 2.42.0
> >>
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >Linux GPU Platform Enablement
> >Intel Corporation