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

[Lucas De Marchi]

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.


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.


>
> - 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