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

Daniel Vetter
2015-10-09T07:31:52Z

On Thu, Oct 08, 2015 at 01:19:27PM -0700, Jesse Barnes wrote:
> 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:

Maybe I'm mixing things up again, but with the evict-stolen-to-shmem
patch we have in the stolen series the tooling should be there already.
Tricky bit will be to make it race-free, which is probably impossible
(within a reasonable amount of effort).

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

Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation