| Message ID | 1443023547-19896-4-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 23, 2015 at 12:52:23PM -0300, 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. fbcon is just a small part of the problem, we can trivially fill stolen with kernel objects even before we let userspace at it. I agree that being able to prioritise allocation to HW functions is good, but it is not that hard to write an eviction + migration pass - given that we already have large chunks of that written. The only issue is that (at least the sketch I have in mind) will only evict objects so if we have fragmentation caused by HW functions, allocations can still fail. -Chris
On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote: > On Wed, Sep 23, 2015 at 12:52:23PM -0300, 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. > > fbcon is just a small part of the problem, we can trivially fill stolen > with kernel objects even before we let userspace at it. I agree that being > able to prioritise allocation to HW functions is good, but it is not that hard > to write an eviction + migration pass - given that we already have large > chunks of that written. The only issue is that (at least the sketch I > have in mind) will only evict objects so if we have fragmentation > caused by HW functions, allocations can still fail. Problem with fbcon is that we can migrate it (well we could do _really_ crazy stuff with a vmalloc are, but that seems pointless). So I think we still need this hack no matter what. -Daniel
On Mon, Sep 28, 2015 at 10:54:59AM +0200, Daniel Vetter wrote: > On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote: > > On Wed, Sep 23, 2015 at 12:52:23PM -0300, 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. > > > > fbcon is just a small part of the problem, we can trivially fill stolen > > with kernel objects even before we let userspace at it. I agree that being > > able to prioritise allocation to HW functions is good, but it is not that hard > > to write an eviction + migration pass - given that we already have large > > chunks of that written. The only issue is that (at least the sketch I > > have in mind) will only evict objects so if we have fragmentation > > caused by HW functions, allocations can still fail. > > Problem with fbcon is that we can migrate it (well we could do _really_ Can or can't? The screen.base pointer is an iomap so to migrate it we just need to point the PTE elsewhere. We can't combine a vmalloc arena and stolen anyway :| -Chris
On Mon, Sep 28, 2015 at 10:09:18AM +0100, Chris Wilson wrote: > On Mon, Sep 28, 2015 at 10:54:59AM +0200, Daniel Vetter wrote: > > On Wed, Sep 23, 2015 at 05:54:25PM +0100, Chris Wilson wrote: > > > On Wed, Sep 23, 2015 at 12:52:23PM -0300, 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. > > > > > > fbcon is just a small part of the problem, we can trivially fill stolen > > > with kernel objects even before we let userspace at it. I agree that being > > > able to prioritise allocation to HW functions is good, but it is not that hard > > > to write an eviction + migration pass - given that we already have large > > > chunks of that written. The only issue is that (at least the sketch I > > > have in mind) will only evict objects so if we have fragmentation > > > caused by HW functions, allocations can still fail. > > > > Problem with fbcon is that we can migrate it (well we could do _really_ > > Can or can't? The screen.base pointer is an iomap so to migrate it we > just need to point the PTE elsewhere. We can't combine a vmalloc arena > and stolen anyway :| I meant can't. And the vma thing would have been to virtualize the gtt offset so that we can move that one around (I was too lazy to look up). But since we could exchange the gtt ptes that's not actually required, I mixed that up with evicting fbcon from gtt. In short I made a mess ;-) -Daniel
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
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
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) {
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(-)