Re: [PATCH 3/7] drm/i915: don't allocate fbcon from stolen memory if it's too big

Jesse Barnes
2015-10-08T20:18:49Z

On 09/23/2015 08:52 AM, Paulo Zanoni wrote:
> Technology has evolved and now we have eDP panels with 3200x1800
> resolution. In the meantime, the BIOS guys didn't change the default
> 32mb for stolen memory. On top of that, we can't assume our users will
> be able to increase the default stolen memory size to more than 32mb -
> I'm not even sure all BIOSes allow that.
>
> So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
> to the BDW/SKL restriction of not using the last 8mb of stolen memory,
> all that's left for FBC is 2mb! Since fbcon is not the coolest feature
> ever, I think it's better to save our precious stolen resource to FBC
> and the other guys.
>
> On the other hand, we really want to use as much stolen memory as
> possible, since on some older systems the stolen memory may be a
> considerable percentage of the total available memory.
>
> This patch tries to achieve a little balance using a simple heuristic:
> if the fbcon wants more than half of the available stolen memory,
> don't use stolen memory in order to leave some for FBC and the other
> features.
>
> The long term plan should be to implement a way to set priorities for
> stolen memory allocation and then evict low priority users when the
> high priority ones need the memory. While we still don't have that,
> let's try to make FBC usable with the simple solution.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> drivers/gpu/drm/i915/intel_fbdev.c | 10 ++++++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2a1fab3..24b8a72 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2486,6 +2486,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> struct intel_initial_plane_config *plane_config)
> {
> struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_i915_gem_object *obj = NULL;
> struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> struct drm_framebuffer *fb = &plane_config->fb->base;
> @@ -2498,6 +2499,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> if (plane_config->size == 0)
> return false;
>
> + /* 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 (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
> + return false;
> +
> obj = i915_gem_object_create_stolen_for_preallocated(dev,
> base_aligned,
> base_aligned,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..4fd5fdf 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -121,8 +121,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> container_of(helper, struct intel_fbdev, helper);
> struct drm_framebuffer *fb;
> struct drm_device *dev = helper->dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_mode_fb_cmd2 mode_cmd = {};
> - struct drm_i915_gem_object *obj;
> + struct drm_i915_gem_object *obj = NULL;
> int size, ret;
>
> /* we don't do packed 24bpp */
> @@ -139,7 +140,12 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>
> size = mode_cmd.pitches[0] * mode_cmd.height;
> size = PAGE_ALIGN(size);
> - obj = i915_gem_object_create_stolen(dev, size);
> +
> + /* 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 (size * 2 < dev_priv->gtt.stolen_usable_size)
> + obj = i915_gem_object_create_stolen(dev, size);
> if (obj == NULL)
> obj = i915_gem_alloc_object(dev, size);
> if (!obj) {
>

I agree with Chris that we should make this smarter too, but I don't
think this hurts in the meantime, so:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Might be nice to macro-ize the size comparison too, both for readability
and to change our threshold in one place if we ever need to.

Thanks,
Jesse