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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 27 Mar 2019 10:21:15 +0100
Daniel Vetter <daniel@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@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.

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.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev