Re: [igt-dev] [PATCH i-g-t 5/9] tests/kms_lease: test implicit primary plane usage in setcrtc ioctl

[Daniel Vetter <daniel@ffwll.ch>]

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@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@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.

> 	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.

> Looks good otherwise.

Cleared up with my comments above and r-b for the entire thing (with the
commit message fixed)?
-Daniel

> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> > +	close(mcl.fd);
> > +
> > +	cleanup_crtc(&data->master,
> > +		     connector_id_to_output(&data->master.display, data->connector_id));
> > +}
> > +
> >  /* Test listing lessees */
> >  static void lessee_list(data_t *data)
> >  {
> > @@ -1086,6 +1145,7 @@ igt_main
> >  		{ "lease_invalid_crtc", lease_invalid_crtc },
> >  		{ "lease_invalid_plane", lease_invalid_plane },
> >  		{ "page_flip_implicit_plane", page_flip_implicit_plane },
> > +		{ "setcrtc_implicit_plane", setcrtc_implicit_plane },
> >  		{ }
> >  	}, *f;
> >  
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev