mbox series

[igt,v2,0/3] Initial igt tests for drm/msm ioctls

Message ID 20210825233139.1066504-1-robdclark@gmail.com
Headers show
Series Initial igt tests for drm/msm ioctls | expand

Message

Rob Clark Aug. 25, 2021, 11:31 p.m. UTC
From: Rob Clark <robdclark@chromium.org>

Add an initial set of tests for the gpu SUBMIT ioctl.  There is
plenty more we can add, but need to start somewhere.

v2: add msm_device::gen and drop igt_msm_pipe_gen().. any test
    that needs to build even trivial cmdstream will need this
    so make it part of the util helper code

Rob Clark (3):
  drmtest: Add DRIVER_MSM support
  msm: Add helper library
  msm: Add submit ioctl tests

 lib/drmtest.c      |   3 +
 lib/drmtest.h      |   1 +
 lib/igt_msm.c      | 171 +++++++++++++++++++++++++++++++++++++++++
 lib/igt_msm.h      | 119 +++++++++++++++++++++++++++++
 lib/meson.build    |   1 +
 tests/meson.build  |   1 +
 tests/msm_submit.c | 186 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 482 insertions(+)
 create mode 100644 lib/igt_msm.c
 create mode 100644 lib/igt_msm.h
 create mode 100644 tests/msm_submit.c

Comments

Petri Latvala Aug. 26, 2021, 5:31 a.m. UTC | #1
On Wed, Aug 25, 2021 at 04:31:39PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>

> 

> Add an initial set of tests for the submit ioctl.

> 

> Signed-off-by: Rob Clark <robdclark@chromium.org>

> ---

>  tests/meson.build  |   1 +

>  tests/msm_submit.c | 186 +++++++++++++++++++++++++++++++++++++++++++++

>  2 files changed, 187 insertions(+)

>  create mode 100644 tests/msm_submit.c

> 

> diff --git a/tests/meson.build b/tests/meson.build

> index 1bdfddbb..ff7c709a 100644

> --- a/tests/meson.build

> +++ b/tests/meson.build

> @@ -107,6 +107,7 @@ test_progs = [

>  	'vc4_wait_seqno',

>  	'vgem_basic',

>  	'vgem_slow',

> +	'msm_submit',

>  ]

>  

>  i915_progs = [

> diff --git a/tests/msm_submit.c b/tests/msm_submit.c

> new file mode 100644

> index 00000000..da93c574

> --- /dev/null

> +++ b/tests/msm_submit.c

> @@ -0,0 +1,186 @@

> +/*

> + * Copyright © 2021 Google, Inc.

> + *

> + * Permission is hereby granted, free of charge, to any person obtaining a

> + * copy of this software and associated documentation files (the "Software"),

> + * to deal in the Software without restriction, including without limitation

> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,

> + * and/or sell copies of the Software, and to permit persons to whom the

> + * Software is furnished to do so, subject to the following conditions:

> + *

> + * The above copyright notice and this permission notice (including the next

> + * paragraph) shall be included in all copies or substantial portions of the

> + * Software.

> + *

> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL

> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING

> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS

> + * IN THE SOFTWARE.

> + */

> +

> +#include "igt.h"

> +#include "igt_msm.h"

> +

> +igt_main

> +{

> +	struct msm_device *dev;

> +	struct msm_pipe *pipe;

> +	struct msm_bo *a, *b;

> +

> +	igt_fixture {

> +		dev = igt_msm_dev_open();


What I replied on 2/3 applies here: If opening the device fails,
igt_msm_dev_open() does not return and 'dev' is left uninitialized,
those other pointers likewise. Leading to...

> +		pipe = igt_msm_pipe_open(dev, 0);

> +		a = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> +		b = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> +	}

> +

> +	igt_subtest("empty-submit") {

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = pipe->submitqueue_id,

> +		};

> +		do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> +	}

> +

> +	igt_subtest("invalid-queue-submit") {

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = 0x1234,

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, ENOENT);

> +	}

> +

> +	igt_subtest("invalid-flags-submit") {

> +		struct drm_msm_gem_submit req = {

> +				.flags   = 0x1234,

> +				.queueid = pipe->submitqueue_id,

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> +	}

> +

> +	igt_subtest("invalid-in-fence-submit") {

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe | MSM_SUBMIT_FENCE_FD_IN,

> +				.queueid = pipe->submitqueue_id,

> +				.fence_fd = dev->fd,  /* This is not a fence fd! */

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> +	}

> +

> +	igt_subtest("invalid-duplicate-bo-submit") {

> +		struct drm_msm_gem_submit_bo bos[] = {

> +			[0] = {

> +				.handle     = a->handle,

> +				.flags      = MSM_SUBMIT_BO_READ,

> +			},

> +			[1] = {

> +				.handle     = b->handle,

> +				.flags      = MSM_SUBMIT_BO_READ,

> +			},

> +			[2] = {

> +				/* this is invalid.. there should not be two entries

> +				 * for the same bo, instead a single entry w/ all

> +				 * usage flags OR'd together should be used.  Kernel

> +				 * should catch this, and return an error code after

> +				 * cleaning up properly (not leaking any bo's)

> +				 */

> +				.handle     = a->handle,

> +				.flags      = MSM_SUBMIT_BO_WRITE,

> +			},

> +		};

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = pipe->submitqueue_id,

> +				.nr_bos  = ARRAY_SIZE(bos),

> +				.bos     = VOID2U64(bos),

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> +	}

> +

> +	igt_subtest("invalid-cmd-idx-submit") {

> +		struct drm_msm_gem_submit_cmd cmds[] = {

> +			[0] = {

> +				.type       = MSM_SUBMIT_CMD_BUF,

> +				.submit_idx = 0,      /* bos[0] does not exist */

> +				.size       = 4 * 4,  /* 4 dwords in cmdbuf */

> +			},

> +		};

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = pipe->submitqueue_id,

> +				.nr_cmds    = ARRAY_SIZE(cmds),

> +				.cmds       = VOID2U64(cmds),

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> +	}

> +

> +	igt_subtest("invalid-cmd-type-submit") {

> +		struct drm_msm_gem_submit_bo bos[] = {

> +			[0] = {

> +				.handle     = a->handle,

> +				.flags      = MSM_SUBMIT_BO_READ,

> +			},

> +		};

> +		struct drm_msm_gem_submit_cmd cmds[] = {

> +			[0] = {

> +				.type       = 0x1234,

> +				.submit_idx = 0,

> +				.size       = 4 * 4,  /* 4 dwords in cmdbuf */

> +			},

> +		};

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = pipe->submitqueue_id,

> +				.nr_cmds    = ARRAY_SIZE(cmds),

> +				.cmds       = VOID2U64(cmds),

> +				.nr_bos  = ARRAY_SIZE(bos),

> +				.bos     = VOID2U64(bos),

> +		};

> +		do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> +	}

> +

> +	igt_subtest("valid-submit") {

> +		struct drm_msm_gem_submit_bo bos[] = {

> +			[0] = {

> +				.handle     = a->handle,

> +				.flags      = MSM_SUBMIT_BO_READ,

> +			},

> +		};

> +		struct drm_msm_gem_submit_cmd cmds[] = {

> +			[0] = {

> +				.type       = MSM_SUBMIT_CMD_BUF,

> +				.submit_idx = 0,

> +				.size       = 4 * 4,  /* 4 dwords in cmdbuf */

> +			},

> +		};

> +		struct drm_msm_gem_submit req = {

> +				.flags   = pipe->pipe,

> +				.queueid = pipe->submitqueue_id,

> +				.nr_cmds    = ARRAY_SIZE(cmds),

> +				.cmds       = VOID2U64(cmds),

> +				.nr_bos  = ARRAY_SIZE(bos),

> +				.bos     = VOID2U64(bos),

> +		};

> +		uint32_t *cmdstream = igt_msm_bo_map(a);

> +		if (dev->gen >= 5) {

> +			*(cmdstream++) = pm4_pkt7_hdr(CP_NOP, 3);

> +		} else {

> +			*(cmdstream++) = pm4_pkt3_hdr(CP_NOP, 3);

> +		}

> +		*(cmdstream++) = 0;

> +		*(cmdstream++) = 0;

> +		*(cmdstream++) = 0;

> +

> +		do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> +	}

> +

> +	igt_fixture {

> +		igt_msm_bo_free(a);

> +		igt_msm_bo_free(b);

> +		igt_msm_pipe_close(pipe);

> +		igt_msm_dev_close(dev);


... crashes in here.


-- 
Petri Latvala
Rob Clark Aug. 26, 2021, 3:37 p.m. UTC | #2
On Wed, Aug 25, 2021 at 10:28 PM Petri Latvala <petri.latvala@intel.com> wrote:
>

> On Wed, Aug 25, 2021 at 04:31:39PM -0700, Rob Clark wrote:

> > From: Rob Clark <robdclark@chromium.org>

> >

> > Add an initial set of tests for the submit ioctl.

> >

> > Signed-off-by: Rob Clark <robdclark@chromium.org>

> > ---

> >  tests/meson.build  |   1 +

> >  tests/msm_submit.c | 186 +++++++++++++++++++++++++++++++++++++++++++++

> >  2 files changed, 187 insertions(+)

> >  create mode 100644 tests/msm_submit.c

> >

> > diff --git a/tests/meson.build b/tests/meson.build

> > index 1bdfddbb..ff7c709a 100644

> > --- a/tests/meson.build

> > +++ b/tests/meson.build

> > @@ -107,6 +107,7 @@ test_progs = [

> >       'vc4_wait_seqno',

> >       'vgem_basic',

> >       'vgem_slow',

> > +     'msm_submit',

> >  ]

> >

> >  i915_progs = [

> > diff --git a/tests/msm_submit.c b/tests/msm_submit.c

> > new file mode 100644

> > index 00000000..da93c574

> > --- /dev/null

> > +++ b/tests/msm_submit.c

> > @@ -0,0 +1,186 @@

> > +/*

> > + * Copyright © 2021 Google, Inc.

> > + *

> > + * Permission is hereby granted, free of charge, to any person obtaining a

> > + * copy of this software and associated documentation files (the "Software"),

> > + * to deal in the Software without restriction, including without limitation

> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,

> > + * and/or sell copies of the Software, and to permit persons to whom the

> > + * Software is furnished to do so, subject to the following conditions:

> > + *

> > + * The above copyright notice and this permission notice (including the next

> > + * paragraph) shall be included in all copies or substantial portions of the

> > + * Software.

> > + *

> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL

> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING

> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS

> > + * IN THE SOFTWARE.

> > + */

> > +

> > +#include "igt.h"

> > +#include "igt_msm.h"

> > +

> > +igt_main

> > +{

> > +     struct msm_device *dev;

> > +     struct msm_pipe *pipe;

> > +     struct msm_bo *a, *b;

> > +

> > +     igt_fixture {

> > +             dev = igt_msm_dev_open();

>

> What I replied on 2/3 applies here: If opening the device fails,

> igt_msm_dev_open() does not return and 'dev' is left uninitialized,

> those other pointers likewise. Leading to...

>

> > +             pipe = igt_msm_pipe_open(dev, 0);

> > +             a = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> > +             b = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> > +     }

> > +

> > +     igt_subtest("empty-submit") {

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = pipe->submitqueue_id,

> > +             };

> > +             do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> > +     }

> > +

> > +     igt_subtest("invalid-queue-submit") {

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = 0x1234,

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, ENOENT);

> > +     }

> > +

> > +     igt_subtest("invalid-flags-submit") {

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = 0x1234,

> > +                             .queueid = pipe->submitqueue_id,

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > +     }

> > +

> > +     igt_subtest("invalid-in-fence-submit") {

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe | MSM_SUBMIT_FENCE_FD_IN,

> > +                             .queueid = pipe->submitqueue_id,

> > +                             .fence_fd = dev->fd,  /* This is not a fence fd! */

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > +     }

> > +

> > +     igt_subtest("invalid-duplicate-bo-submit") {

> > +             struct drm_msm_gem_submit_bo bos[] = {

> > +                     [0] = {

> > +                             .handle     = a->handle,

> > +                             .flags      = MSM_SUBMIT_BO_READ,

> > +                     },

> > +                     [1] = {

> > +                             .handle     = b->handle,

> > +                             .flags      = MSM_SUBMIT_BO_READ,

> > +                     },

> > +                     [2] = {

> > +                             /* this is invalid.. there should not be two entries

> > +                              * for the same bo, instead a single entry w/ all

> > +                              * usage flags OR'd together should be used.  Kernel

> > +                              * should catch this, and return an error code after

> > +                              * cleaning up properly (not leaking any bo's)

> > +                              */

> > +                             .handle     = a->handle,

> > +                             .flags      = MSM_SUBMIT_BO_WRITE,

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = pipe->submitqueue_id,

> > +                             .nr_bos  = ARRAY_SIZE(bos),

> > +                             .bos     = VOID2U64(bos),

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > +     }

> > +

> > +     igt_subtest("invalid-cmd-idx-submit") {

> > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > +                     [0] = {

> > +                             .type       = MSM_SUBMIT_CMD_BUF,

> > +                             .submit_idx = 0,      /* bos[0] does not exist */

> > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = pipe->submitqueue_id,

> > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > +                             .cmds       = VOID2U64(cmds),

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > +     }

> > +

> > +     igt_subtest("invalid-cmd-type-submit") {

> > +             struct drm_msm_gem_submit_bo bos[] = {

> > +                     [0] = {

> > +                             .handle     = a->handle,

> > +                             .flags      = MSM_SUBMIT_BO_READ,

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > +                     [0] = {

> > +                             .type       = 0x1234,

> > +                             .submit_idx = 0,

> > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = pipe->submitqueue_id,

> > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > +                             .cmds       = VOID2U64(cmds),

> > +                             .nr_bos  = ARRAY_SIZE(bos),

> > +                             .bos     = VOID2U64(bos),

> > +             };

> > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > +     }

> > +

> > +     igt_subtest("valid-submit") {

> > +             struct drm_msm_gem_submit_bo bos[] = {

> > +                     [0] = {

> > +                             .handle     = a->handle,

> > +                             .flags      = MSM_SUBMIT_BO_READ,

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > +                     [0] = {

> > +                             .type       = MSM_SUBMIT_CMD_BUF,

> > +                             .submit_idx = 0,

> > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > +                     },

> > +             };

> > +             struct drm_msm_gem_submit req = {

> > +                             .flags   = pipe->pipe,

> > +                             .queueid = pipe->submitqueue_id,

> > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > +                             .cmds       = VOID2U64(cmds),

> > +                             .nr_bos  = ARRAY_SIZE(bos),

> > +                             .bos     = VOID2U64(bos),

> > +             };

> > +             uint32_t *cmdstream = igt_msm_bo_map(a);

> > +             if (dev->gen >= 5) {

> > +                     *(cmdstream++) = pm4_pkt7_hdr(CP_NOP, 3);

> > +             } else {

> > +                     *(cmdstream++) = pm4_pkt3_hdr(CP_NOP, 3);

> > +             }

> > +             *(cmdstream++) = 0;

> > +             *(cmdstream++) = 0;

> > +             *(cmdstream++) = 0;

> > +

> > +             do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> > +     }

> > +

> > +     igt_fixture {

> > +             igt_msm_bo_free(a);

> > +             igt_msm_bo_free(b);

> > +             igt_msm_pipe_close(pipe);

> > +             igt_msm_dev_close(dev);

>

> ... crashes in here.

>


I did test this on intel as well, and it skips properly.. I think the
setjmp/longjmp magic just bails completely out so we never try to run
the cleanup?

BR,
-R
Petri Latvala Aug. 27, 2021, 5:37 a.m. UTC | #3
On Thu, Aug 26, 2021 at 08:37:19AM -0700, Rob Clark wrote:
> On Wed, Aug 25, 2021 at 10:28 PM Petri Latvala <petri.latvala@intel.com> wrote:

> >

> > On Wed, Aug 25, 2021 at 04:31:39PM -0700, Rob Clark wrote:

> > > From: Rob Clark <robdclark@chromium.org>

> > >

> > > Add an initial set of tests for the submit ioctl.

> > >

> > > Signed-off-by: Rob Clark <robdclark@chromium.org>

> > > ---

> > >  tests/meson.build  |   1 +

> > >  tests/msm_submit.c | 186 +++++++++++++++++++++++++++++++++++++++++++++

> > >  2 files changed, 187 insertions(+)

> > >  create mode 100644 tests/msm_submit.c

> > >

> > > diff --git a/tests/meson.build b/tests/meson.build

> > > index 1bdfddbb..ff7c709a 100644

> > > --- a/tests/meson.build

> > > +++ b/tests/meson.build

> > > @@ -107,6 +107,7 @@ test_progs = [

> > >       'vc4_wait_seqno',

> > >       'vgem_basic',

> > >       'vgem_slow',

> > > +     'msm_submit',

> > >  ]

> > >

> > >  i915_progs = [

> > > diff --git a/tests/msm_submit.c b/tests/msm_submit.c

> > > new file mode 100644

> > > index 00000000..da93c574

> > > --- /dev/null

> > > +++ b/tests/msm_submit.c

> > > @@ -0,0 +1,186 @@

> > > +/*

> > > + * Copyright © 2021 Google, Inc.

> > > + *

> > > + * Permission is hereby granted, free of charge, to any person obtaining a

> > > + * copy of this software and associated documentation files (the "Software"),

> > > + * to deal in the Software without restriction, including without limitation

> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,

> > > + * and/or sell copies of the Software, and to permit persons to whom the

> > > + * Software is furnished to do so, subject to the following conditions:

> > > + *

> > > + * The above copyright notice and this permission notice (including the next

> > > + * paragraph) shall be included in all copies or substantial portions of the

> > > + * Software.

> > > + *

> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR

> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,

> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL

> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER

> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING

> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS

> > > + * IN THE SOFTWARE.

> > > + */

> > > +

> > > +#include "igt.h"

> > > +#include "igt_msm.h"

> > > +

> > > +igt_main

> > > +{

> > > +     struct msm_device *dev;

> > > +     struct msm_pipe *pipe;

> > > +     struct msm_bo *a, *b;

> > > +

> > > +     igt_fixture {

> > > +             dev = igt_msm_dev_open();

> >

> > What I replied on 2/3 applies here: If opening the device fails,

> > igt_msm_dev_open() does not return and 'dev' is left uninitialized,

> > those other pointers likewise. Leading to...

> >

> > > +             pipe = igt_msm_pipe_open(dev, 0);

> > > +             a = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> > > +             b = igt_msm_bo_new(dev, 0x1000, MSM_BO_WC);

> > > +     }

> > > +

> > > +     igt_subtest("empty-submit") {

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +             };

> > > +             do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-queue-submit") {

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = 0x1234,

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, ENOENT);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-flags-submit") {

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = 0x1234,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-in-fence-submit") {

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe | MSM_SUBMIT_FENCE_FD_IN,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +                             .fence_fd = dev->fd,  /* This is not a fence fd! */

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-duplicate-bo-submit") {

> > > +             struct drm_msm_gem_submit_bo bos[] = {

> > > +                     [0] = {

> > > +                             .handle     = a->handle,

> > > +                             .flags      = MSM_SUBMIT_BO_READ,

> > > +                     },

> > > +                     [1] = {

> > > +                             .handle     = b->handle,

> > > +                             .flags      = MSM_SUBMIT_BO_READ,

> > > +                     },

> > > +                     [2] = {

> > > +                             /* this is invalid.. there should not be two entries

> > > +                              * for the same bo, instead a single entry w/ all

> > > +                              * usage flags OR'd together should be used.  Kernel

> > > +                              * should catch this, and return an error code after

> > > +                              * cleaning up properly (not leaking any bo's)

> > > +                              */

> > > +                             .handle     = a->handle,

> > > +                             .flags      = MSM_SUBMIT_BO_WRITE,

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +                             .nr_bos  = ARRAY_SIZE(bos),

> > > +                             .bos     = VOID2U64(bos),

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-cmd-idx-submit") {

> > > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > > +                     [0] = {

> > > +                             .type       = MSM_SUBMIT_CMD_BUF,

> > > +                             .submit_idx = 0,      /* bos[0] does not exist */

> > > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > > +                             .cmds       = VOID2U64(cmds),

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > > +     }

> > > +

> > > +     igt_subtest("invalid-cmd-type-submit") {

> > > +             struct drm_msm_gem_submit_bo bos[] = {

> > > +                     [0] = {

> > > +                             .handle     = a->handle,

> > > +                             .flags      = MSM_SUBMIT_BO_READ,

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > > +                     [0] = {

> > > +                             .type       = 0x1234,

> > > +                             .submit_idx = 0,

> > > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > > +                             .cmds       = VOID2U64(cmds),

> > > +                             .nr_bos  = ARRAY_SIZE(bos),

> > > +                             .bos     = VOID2U64(bos),

> > > +             };

> > > +             do_ioctl_err(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req, EINVAL);

> > > +     }

> > > +

> > > +     igt_subtest("valid-submit") {

> > > +             struct drm_msm_gem_submit_bo bos[] = {

> > > +                     [0] = {

> > > +                             .handle     = a->handle,

> > > +                             .flags      = MSM_SUBMIT_BO_READ,

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit_cmd cmds[] = {

> > > +                     [0] = {

> > > +                             .type       = MSM_SUBMIT_CMD_BUF,

> > > +                             .submit_idx = 0,

> > > +                             .size       = 4 * 4,  /* 4 dwords in cmdbuf */

> > > +                     },

> > > +             };

> > > +             struct drm_msm_gem_submit req = {

> > > +                             .flags   = pipe->pipe,

> > > +                             .queueid = pipe->submitqueue_id,

> > > +                             .nr_cmds    = ARRAY_SIZE(cmds),

> > > +                             .cmds       = VOID2U64(cmds),

> > > +                             .nr_bos  = ARRAY_SIZE(bos),

> > > +                             .bos     = VOID2U64(bos),

> > > +             };

> > > +             uint32_t *cmdstream = igt_msm_bo_map(a);

> > > +             if (dev->gen >= 5) {

> > > +                     *(cmdstream++) = pm4_pkt7_hdr(CP_NOP, 3);

> > > +             } else {

> > > +                     *(cmdstream++) = pm4_pkt3_hdr(CP_NOP, 3);

> > > +             }

> > > +             *(cmdstream++) = 0;

> > > +             *(cmdstream++) = 0;

> > > +             *(cmdstream++) = 0;

> > > +

> > > +             do_ioctl(dev->fd, DRM_IOCTL_MSM_GEM_SUBMIT, &req);

> > > +     }

> > > +

> > > +     igt_fixture {

> > > +             igt_msm_bo_free(a);

> > > +             igt_msm_bo_free(b);

> > > +             igt_msm_pipe_close(pipe);

> > > +             igt_msm_dev_close(dev);

> >

> > ... crashes in here.

> >

> 

> I did test this on intel as well, and it skips properly.. I think the

> setjmp/longjmp magic just bails completely out so we never try to run

> the cleanup?


Ah, indeed, skip_henceforth prevents entering the latter fixture.

It still looks scary, can I implore to add some defensive layers with
NULL-inits and handling of NULLs in the functions? It might not crash
today...


-- 
Petri Latvala