[igt-dev] [PATCH i-g-t 5/9] tests/kms_lease: test implicit primary plane usage in setcrtc ioctl
Daniel Vetter
daniel at ffwll.ch
Wed Mar 27 12:14:58 UTC 2019
On Wed, Mar 27, 2019 at 11:10 AM Boris Brezillon
<boris.brezillon at collabora.com> wrote:
>
> On Wed, 27 Mar 2019 10:21:15 +0100
> Daniel Vetter <daniel at ffwll.ch> wrote:
>
> > On Wed, Mar 27, 2019 at 09:26:23AM +0100, Boris Brezillon wrote:
> > > On Thu, 28 Feb 2019 15:19:14 +0100
> > > Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > >
> > > > Again we need to make sure that a lease can't use planes it's not
> > > > allowed to through legacy ioctls. SetCrtc is a bit more tricky, since
> > >
> > > ^ "to use through"?
> >
> > Yup.
> >
> > >
> > > > we should still allow to shut down a CRTC, e.g. when we're allowed to
> > > > use other planes on that CRTC.
> > > >
> > > > Current kernels fail this.
> > > >
> > > > Cc: Keith Packard <keithp at keithp.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > ---
> > > > tests/kms_lease.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 60 insertions(+)
> > > >
> > > > diff --git a/tests/kms_lease.c b/tests/kms_lease.c
> > > > index 6453c7beafd8..c11d37aeac7a 100644
> > > > --- a/tests/kms_lease.c
> > > > +++ b/tests/kms_lease.c
> > > > @@ -372,6 +372,65 @@ static void page_flip_implicit_plane(data_t *data)
> > > > connector_id_to_output(&data->master.display, data->connector_id));
> > > > }
> > > >
> > > > +static void setcrtc_implicit_plane(data_t *data)
> > > > +{
> > > > + uint32_t object_ids[3];
> > > > + struct local_drm_mode_create_lease mcl;
> > > > + drmModePlaneRes *plane_resources;
> > > > + uint32_t wrong_plane_id = 0;
> > > > + igt_output_t *output =
> > > > + connector_id_to_output(&data->master.display,
> > > > + data->connector_id);
> > > > + drmModeModeInfo *mode = igt_output_get_mode(output);
> > > > + int i;
> > > > +
> > > > + /* find a plane which isn't the primary one for us */
> > > > + plane_resources = drmModeGetPlaneResources(data->master.fd);
> > > > + for (i = 0; i < plane_resources->count_planes; i++) {
> > > > + if (plane_resources->planes[i] != data->plane_id) {
> > > > + wrong_plane_id = plane_resources->planes[i];
> > > > + break;
> > > > + }
> > > > + }
> > > > + drmModeFreePlaneResources(plane_resources);
> > > > + igt_require(wrong_plane_id);
> > >
> > > Maybe we could add an helper to retrieve wrong_plane_id instead of
> > > duplicating the code.
> >
> > Hm, where is it duplicated? The other very similar loop looks for the
> > worng connector, and there's a few other loops to look for the right
> > plane/connector. I think they're all different.
> > >
> > > > +
> > > > + mcl.object_ids = (uint64_t) (uintptr_t) &object_ids[0];
> > > > + mcl.object_count = 0;
> > > > + mcl.flags = 0;
> > > > +
> > > > + object_ids[mcl.object_count++] = data->connector_id;
> > > > + object_ids[mcl.object_count++] = data->crtc_id;
> > > > +
> > > > + drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> > > > + do_or_die(create_lease(data->master.fd, &mcl));
> > > > + drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > > > +
> > > > + /* Set a mode on the leased output */
> > > > + igt_assert_eq(0, prepare_crtc(&data->master, data->connector_id, data->crtc_id));
> > >
> > > Same goes for those create-lease+prepare-CRTC steps.
> >
> > There's a helper for the common case, but for these "wrong" leases there's
> > always subtle differences. A helper would be be about as much code as just
> > open-coding it. Maybe we could move the UNIVERSAL_PLANES as a mode flag
> > into create_lease, but given how often I screamed at my tests for not
> > working because I got that one wrong, I think it's better to be explicit.
> > >
> > > > +
> > > > + /* sanity check */
> > > > + do_or_die(drmModeSetCrtc(data->master.fd, data->crtc_id, -1,
> > > > + 0, 0, object_ids, 1, mode));
> > > > + do_or_die(drmModeSetCrtc(mcl.fd, data->crtc_id, -1,
> > > > + 0, 0, object_ids, 1, mode));
> > > > + close(mcl.fd);
> > > > +
> > > > + object_ids[mcl.object_count++] = wrong_plane_id;
> > > > + do_or_die(create_lease(data->master.fd, &mcl));
> > > > +
> > > > + igt_assert_eq(drmModeSetCrtc(mcl.fd, data->crtc_id, -1,
> > > > + 0, 0, object_ids, 1, mode),
> > > > + -EACCES);
> > > > + /* make sure we are allowed to turn the CRTC off */
> > > > + do_or_die(drmModeSetCrtc(mcl.fd, data->crtc_id,
> > > > + 0, 0, 0, NULL, 0, NULL));
> > >
> > > Do we actually test the case where the lessor has setup the CRTC with a
> > > plane that's not accessible by the lesse. I think that's what you were
> > > trying to test here, but I don't see where it's done in the code.
> > > Shouldn't we have something like:
> > >
> > > do_or_die(drmModeSetCrtc(data->master.fd, data->crtc_id, -1,
> > > 0, 0, object_ids, 1, mode));
> >
> > Sanity check above does exactly that.
>
> Yes, but wrong_plane_id is not part of the object list in your sanity
> check, is it?
>
> >
> > > igt_assert_eq(0, drmModeSetCrtc(mcl.fd, data->crtc_id, 0, 0, 0,
> > > NULL, 0, NULL));
> >
> > That's the line right above wrapped in the do_or_die.
>
> Are you sure wrong_plane_id was enabled when you call drmModeSetCrtc()
> to disable the CRTC? If it wasn't, that means you're not really testing
> the 'disable CRTC even if other planes I don't own are still attached to
> it'. But I'm probably missing something.
Ok, I guess what the test does isn't clear:
- a lease allows you to access the objects in your lease, but not any others
- a bunch of legacy ioctl don't just access the crtc that you pass in
explicitly, but also some implicit stuff
- we want to make sure all legacy ioctls also validate for these
implicit objects
For that we need to construct:
- a valid lease, which means it needs to contain at least a plane,
crtc and connector
- but the lease must _not_ contain the primary plane of the crtc we've picked
Hence the dance with wrong_plane_id. That's the only use of
wrong_plane_id, we don't care at all what that plane does nor where it
is attached to, all we need is a plane which isn't the one we care
about, to be able to construct the lease.
Then the actual test verifies:
- You can't enable the crtc (since that also enables the primary plane
and changes it's fb, which isn't in our lease)
- You can still disable the crtc (since that just changes crtc state,
leaving the planes as-is)
- plus a sanity check: we verify both of these work if we use the master fd
Neither of these tests care what wrong_plane_id does, as long as it's
not the primary plane. And becasuse all the access checks must happen
before we touch the hw there's also no need to first enable the crtc
to be able to shut id down. If it's off already the ioctl is a no-op,
but still succeeds/fails in the access checks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the igt-dev
mailing list