[igt-dev] [PATCH i-g-t v4 52/56] tests/kms_vblank: Adopt to use allocator

[Modem, Bhanuprakash]

> From: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>
> Sent: Monday, August 9, 2021 1:56 PM
> To: igt-dev at lists.freedesktop.org
> Cc: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; Latvala, Petri
> <petri.latvala at intel.com>; Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Subject: Re: [PATCH i-g-t v4 52/56] tests/kms_vblank: Adopt to use allocator
> 
> On Fri, Aug 06, 2021 at 03:41:41PM +0200, Zbigniew Kempczyński wrote:
> > From: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> >
> > For newer gens kernel will reject relocations returning -EINVAL
> > so we should just provide the allocator handle to inject the hang.
> >
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >  tests/kms_vblank.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> > index 885b2e2c4..7b7e1372e 100644
> > --- a/tests/kms_vblank.c
> > +++ b/tests/kms_vblank.c
> > @@ -118,6 +118,7 @@ static void run_test(data_t *data, void
> (*testfunc)(data_t *, int, int))
> >  	igt_output_t *output = data->output;
> >  	int fd = display->drm_fd;
> >  	igt_hang_t hang;
> > +	uint64_t ahnd = 0;
> >
> >  	prepare_crtc(data, fd, output);
> >
> > @@ -128,8 +129,11 @@ static void run_test(data_t *data, void
> (*testfunc)(data_t *, int, int))
> >  		 igt_subtest_name(), kmstest_pipe_name(data->pipe),
> >  		 igt_output_name(output));
> >
> > -	if (!(data->flags & NOHANG))
> > -		hang = igt_hang_ring(fd, I915_EXEC_DEFAULT);
> > +	if (!(data->flags & NOHANG)) {
> > +		if (is_i915_device(fd))
> > +			ahnd = get_reloc_ahnd(fd, 0);
> > +		hang = igt_hang_ring_with_ahnd(fd, I915_EXEC_DEFAULT, ahnd);
> > +	}
> 
> What for is_i915_device(fd) check is introduced here? igt_hang_ring() is
> currently
> valid only for i915 so ahnd = get_reloc_ahnd(fd, 0) can be called
> unconditionally
> here. Am I missed something here?
 
We need this is_i915_device(fd) check, because kms_vblank is not an i915
specific. `fd = drm_open_driver_master(DRIVER_ANY);`

As of now, I guess only Intel is using this test, so no one is concerned,
we already floated a initial version of series to move intel specific kms
tests to test/i915 directory, if part of a test is intel specific we must
limit the test by adding a check igt_require_intel() [*].

Yeah, at least in our context we can drop this is_i915_device() check here
and handle it separately. I'll float a new patch with these changes, thanks
for the review.

[*] https://patchwork.freedesktop.org/series/93224/

- Bhanu
  
> 
> >
> >  	if (data->flags & BUSY) {
> >  		union drm_wait_vblank vbl;
> > @@ -166,6 +170,9 @@ static void run_test(data_t *data, void
> (*testfunc)(data_t *, int, int))
> >  	igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
> >  		 igt_subtest_name(), kmstest_pipe_name(data->pipe),
> igt_output_name(output));
> >
> > +	if (is_i915_device(fd))
> > +		put_ahnd(ahnd);
> > +
> 
> For non-i915 machines ahnd will be equal to 0, so put_ahnd() will just return
> and no i915 dependand code will be called. I would rather expect:
> 
> if (!(data->flags & NOHANG))
> 	put_ahnd(ahnd);
> 
> But as I said put_ahnd() with ahnd == 0 does nothing.
> 
> --
> Zbigniew
> 
> 
> >  	/* cleanup what prepare_crtc() has done */
> >  	cleanup_crtc(data, fd, output);
> >  }
> > --
> > 2.26.0
> >