From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good Date: Fri, 16 May 2014 15:13:48 +0300 Message-ID: <20140516121348.GQ27580@intel.com> References: <1400238531-10414-1-git-send-email-oscar.mateo@intel.com> <20140516120421.GP27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id DCA366E2DF for ; Fri, 16 May 2014 05:13:51 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140516120421.GP27580@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: oscar.mateo@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, May 16, 2014 at 03:04:21PM +0300, Ville Syrj=E4l=E4 wrote: > On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.mateo@intel.com wrote: > > From: Oscar Mateo > > = > > This is a "review by igt test" for a bug located in > > i915_gem_object_pin_to_display_plane and fixed by: > > = > > commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3 > > Author: Oscar Mateo > > Date: Fri May 16 11:23:12 2014 +0100 > > = > > drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display > > = > > Otherwise, we do a NULL pointer dereference. > > = > > I've seen this happen while handling an error in > > i915_gem_object_pin_to_display_plane(): > > = > > If i915_gem_object_set_cache_level() fails, we call is_pin_display() > > to handle the error. At this point, the object is still not pinned > > to GGTT and maybe not even bound, so we have to check before we > > dereference its GGTT vma. > > = > > v2: Chris Wilson says restoring the old value is easier, but that > > is_pin_display is useful as a theory of operation. Take the solomon= ic > > decision: at least this way is_pin_display is a little more robust > > (until Chris can kill it off). > > = > > Signed-off-by: Oscar Mateo > > --- > > tests/kms_flip.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++= +++++--- > > 1 file changed, 97 insertions(+), 5 deletions(-) > > = > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c > > index bb4f71d..7296122 100644 > > --- a/tests/kms_flip.c > > +++ b/tests/kms_flip.c > > @@ -74,6 +74,7 @@ > > #define TEST_RPM (1 << 25) > > #define TEST_SUSPEND (1 << 26) > > #define TEST_TS_CONT (1 << 27) > > +#define TEST_BO_TOOBIG (1 << 28) > > = > > #define EVENT_FLIP (1 << 0) > > #define EVENT_VBLANK (1 << 1) > > @@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output = *o) > > } > > } > > = > > +static int create_bigbo_for_fb(int fd, int width, int height, int bpp, > > + bool tiled, uint32_t *gem_handle_ret, > > + unsigned *size_ret, unsigned *stride_ret) > > +{ > > + uint32_t gem_handle; > > + int size, ret =3D 0; > > + unsigned stride; > > + > > + if (tiled) { > > + int v; > > + > > + v =3D width * bpp / 8; > > + for (stride =3D 512; stride < v; stride *=3D 2) > > + ; > > + > > + v =3D stride * height; > > + for (size =3D 1024*1024; size < v; size *=3D 2) > > + ; > > + } else { > > + /* Scan-out has a 64 byte alignment restriction */ > > + stride =3D (width * (bpp / 8) + 63) & ~63; > > + size =3D stride * height; > > + } > > + > > + /* 256 MB is usually the maximum mappable aperture, > > + * (make it 4x times that to ensure failure) */ > > + gem_handle =3D gem_create(fd, 4*256*1024*1024); > > + > > + if (tiled) > > + ret =3D __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride); > > + > > + *stride_ret =3D stride; > > + *size_ret =3D size; > > + *gem_handle_ret =3D gem_handle; > > + > > + return ret; > > +} > > + > > +static unsigned int igt_create_fb_with_bigbo(int fd, int width, int he= ight, > > + uint32_t format, bool tiled, > > + struct igt_fb *fb) > > +{ > > + uint32_t handles[4]; > > + uint32_t pitches[4]; > > + uint32_t offsets[4]; > > + uint32_t fb_id; > > + int bpp; > > + int ret; > > + > > + memset(fb, 0, sizeof(*fb)); > > + > > + bpp =3D igt_drm_format_to_bpp(format); > > + ret =3D create_bigbo_for_fb(fd, width, height, bpp, tiled, > > + &fb->gem_handle, &fb->size, &fb->stride); > > + if (ret < 0) > > + return ret; > > + > > + memset(handles, 0, sizeof(handles)); > > + handles[0] =3D fb->gem_handle; > > + memset(pitches, 0, sizeof(pitches)); > > + pitches[0] =3D fb->stride; > > + memset(offsets, 0, sizeof(offsets)); > > + if (drmModeAddFB2(fd, width, height, format, handles, pitches, > > + offsets, &fb_id, 0) < 0) { > > + gem_close(fd, fb->gem_handle); > > + > > + return 0; > > + } > > + > > + fb->width =3D width; > > + fb->height =3D height; > > + fb->tiling =3D tiled; > > + fb->drm_format =3D format; > > + fb->fb_id =3D fb_id; > > + > > + return fb_id; > > +} > = > Could we avoid a bit of code duplication with something like this? > = > create_bo_for_fb(..., bo_size) > { > ... > if (bo_size =3D=3D 0) > bo_size =3D size; > gem_handle =3D gem_create(fd, bo_size); > ... > } > igt_create_fb_with_bo_size(xxx, bo_size) > { > ... > create_bo_for_fb(..., bo_size); > ... > } > igt_create_fb(xxx) > { > return igt_create_fb_with_bo_size(xxx, 0); > } Oh and that gives me an idea for another test: try to create fb with bo_size < size and check that we get back some kind of error. Assuming we don't have such a test already. -- = Ville Syrj=E4l=E4 Intel OTC