Re: [PATCH] drm/i915: Quietly reject attempts to create non-pagealigned stolen objects

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 21, 2015 at 07:45:01PM +0200, Jani Nikula wrote:
> On Wed, 10 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 10, 2014 at 08:17:11AM +0000, Chris Wilson wrote:
> >> This added as a BUG_ON as it considered that no one would ever request
> >> an unaligned object. However, it turns out that some BIOSes will
> >> allocate a scanout that is offset from 0 and not aligned to a page
> >> boundary, and we were passing this through and hitting the BUG_ON during
> >> boot.
> >> 
> >> Quietly reject such a request to reserve the unaligned stolen object and
> >> let the boot continue, restoring previous behaviour (i.e. no BIOS
> >> framebuffer preservation).
> >> 
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86883
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_stolen.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> index 5c616ec2c5c8..a3bc0fa07c6c 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> @@ -646,13 +646,15 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> >>  	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
> >>  			stolen_offset, gtt_offset, size);
> >>  
> >> -	/* KISS and expect everything to be page-aligned */
> >> -	BUG_ON(stolen_offset & 4095);
> >> -	BUG_ON(size & 4095);
> >> -
> >>  	if (WARN_ON(size == 0))
> >>  		return NULL;
> >>  
> >> +	/* KISS and expect everything to be GTT page-aligned */
> >> +	if ((stolen_offset | size) & 4095) {
> >
> > Imo we should stil WARN_ON and fixup up the takeover code to align things
> > properly ...
> 
> Can we fix the regression with Chris' patch and do proper stuff when we
> figure out what we want to do?

Something like the below would imo be the proper bandaid. Untested since I
don't have an affected system. Then we can still (later on, if bored)
recover the offsets properly if ever needed.
-Daniel

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a2045848bd1a..f75bf292285d 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -485,10 +485,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 			stolen_offset, gtt_offset, size);
 
 	/* KISS and expect everything to be page-aligned */
-	BUG_ON(stolen_offset & 4095);
-	BUG_ON(size & 4095);
-
-	if (WARN_ON(size == 0))
+	if (WARN_ON(size == 0 || stolen_offset & 4095 || size & 4095))
 		return NULL;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 86831aec5c0d..733c99d5b671 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2370,13 +2370,17 @@ intel_alloc_plane_obj(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_gem_object *obj = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-	u32 base = plane_config->base;
+	u32 base_aligned = round_down(plane_config->base, PAGE_SIZE);
+	u32 size_aligned = round_up(plane_config->base + plane_config->size,
+				    PAGE_SIZE);
 
 	if (plane_config->size == 0)
 		return false;
 
-	obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
-							     plane_config->size);
+	obj = i915_gem_object_create_stolen_for_preallocated(dev,
+							     base_aligned,
+							     base_aligned,
+							     size_aligned);
 	if (!obj)
 		return false;
 
@@ -6626,7 +6630,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 	aligned_height = intel_fb_align_height(dev, fb->height,
 					       plane_config->tiling);
 
-	plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
+	plane_config->size = fb->pitches[0] * aligned_height;
 
 	DRM_DEBUG_KMS("pipe/plane %c/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
 		      pipe_name(pipe), plane, fb->width, fb->height,
@@ -7663,7 +7667,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	aligned_height = intel_fb_align_height(dev, fb->height,
 					       plane_config->tiling);
 
-	plane_config->size = ALIGN(fb->pitches[0] * aligned_height, PAGE_SIZE);
+	plane_config->size = fb->pitches[0] * aligned_height, PAGE_SIZE;
 
 	DRM_DEBUG_KMS("pipe %c with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
 		      pipe_name(pipe), fb->width, fb->height,
@@ -7754,7 +7758,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 	aligned_height = intel_fb_align_height(dev, fb->height,
 					       plane_config->tiling);
 
-	plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
+	plane_config->size = fb->pitches[0] * aligned_height;
 
 	DRM_DEBUG_KMS("pipe %c with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
 		      pipe_name(pipe), fb->width, fb->height,
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx