Re: [PATCH 16/20] drm/i915/fbdev: Extract intel_fbdev_fb_prefer_stolen()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 09, 2024 at 03:28:15PM -0500, Lucas De Marchi wrote:
> On Fri, Jul 05, 2024 at 05:52:50PM GMT, Ville Syrjälä wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Consolidate the "should we allocate fbdev fb in stolen?"
> >check into a helper function. Makes it easier to change the
> >heuristics without having to change so many places.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 24 ++++++++++++-------
> > drivers/gpu/drm/i915/display/intel_fbdev_fb.h |  5 +++-
> > .../drm/i915/display/intel_plane_initial.c    | 10 +++-----
> > 3 files changed, 23 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >index 497525ef9668..0a6445acb100 100644
> >--- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >+++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> >@@ -11,6 +11,19 @@
> > #include "intel_display_types.h"
> > #include "intel_fbdev_fb.h"
> >
> >+bool intel_fbdev_fb_prefer_stolen(struct intel_display *display,
> >+				  unsigned int size)
> >+{
> >+	struct drm_i915_private *i915 = to_i915(display->drm);
> >+
> >+	/*
> >+	 * 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
> >+	 * features.
> >+	 */
> >+	return i915->dsm.usable_size >= size * 2;
> >+}
> >+
> > struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
> > 					       struct drm_fb_helper_surface_size *sizes)
> > {
> >@@ -42,14 +55,9 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
> > 						  I915_BO_ALLOC_CONTIGUOUS |
> > 						  I915_BO_ALLOC_USER);
> > 	} else {
> >-		/*
> >-		 * 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
> >-		 * features.
> >-		 *
> >-		 * Also skip stolen on MTL as Wa_22018444074 mitigation.
> >-		 */
> >-		if (!(IS_METEORLAKE(dev_priv)) && size * 2 < dev_priv->dsm.usable_size)
> >+		/* skip stolen on MTL as Wa_22018444074 mitigation */
> >+		if (!IS_METEORLAKE(dev_priv) &&
> 
> shouldn't this be inside intel_fbdev_fb_prefer_stolen()?

That would also apply it to the BIOS fb takeover, so change the
behaviour. The correct answer is likely just removing the MTL check
entirely, but I left that out for now to avoid too many functional
changes.

> 
> And also pull the same logic on the xe side a few patches after this.
> 
> Lucas De Marchi
> 
> >+		    intel_fbdev_fb_prefer_stolen(&dev_priv->display, size))
> > 			obj = i915_gem_object_create_stolen(dev_priv, size);
> > 		if (IS_ERR(obj))
> > 			obj = i915_gem_object_create_shmem(dev_priv, size);
> >diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >index 4832fe688fbf..3b9033bd2160 100644
> >--- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >+++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> >@@ -6,16 +6,19 @@
> > #ifndef __INTEL_FBDEV_FB_H__
> > #define __INTEL_FBDEV_FB_H__
> >
> >+#include <linux/types.h>
> >+
> > struct drm_fb_helper;
> > struct drm_fb_helper_surface_size;
> > struct drm_i915_gem_object;
> > struct drm_i915_private;
> > struct fb_info;
> > struct i915_vma;
> >+struct intel_display;
> >
> > struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
> > 					       struct drm_fb_helper_surface_size *sizes);
> > int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info,
> > 			     struct drm_i915_gem_object *obj, struct i915_vma *vma);
> >-
> >+bool intel_fbdev_fb_prefer_stolen(struct intel_display *display, unsigned int size);
> > #endif
> >diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> >index ada1792df5b3..4622bb5f3426 100644
> >--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> >+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> >@@ -11,6 +11,7 @@
> > #include "intel_display.h"
> > #include "intel_display_types.h"
> > #include "intel_fb.h"
> >+#include "intel_fbdev_fb.h"
> > #include "intel_frontbuffer.h"
> > #include "intel_plane_initial.h"
> >
> >@@ -160,15 +161,10 @@ initial_plane_vma(struct drm_i915_private *i915,
> > 			mem->min_page_size);
> > 	size -= base;
> >
> >-	/*
> >-	 * 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
> >-	 * features.
> >-	 */
> > 	if (IS_ENABLED(CONFIG_FRAMEBUFFER_CONSOLE) &&
> > 	    mem == i915->mm.stolen_region &&
> >-	    size * 2 > i915->dsm.usable_size) {
> >-		drm_dbg_kms(&i915->drm, "Initial FB size exceeds half of stolen, discarding\n");
> >+	    !intel_fbdev_fb_prefer_stolen(&i915->display, size)) {
> >+		drm_dbg_kms(&i915->drm, "Initial FB size uses too much stolen, discarding\n");
> > 		return NULL;
> > 	}
> >
> >-- 
> >2.44.2
> >

-- 
Ville Syrjälä
Intel