Re: [igt-dev] [PATCH i-g-t v1] tests/kms_pipe_crc_basic: Support custom CRC sources

[Petri Latvala <petri.latvala@intel.com>]

On Fri, Jun 03, 2022 at 10:06:55AM -0700, Jessica Zhang wrote:
> 
> 
> On 6/3/2022 12:16 AM, Petri Latvala wrote:
> > On Thu, Jun 02, 2022 at 01:41:16PM -0700, Jessica Zhang wrote:
> > > 
> > > 
> > > On 6/2/2022 5:54 AM, Petri Latvala wrote:
> > > > On Tue, May 31, 2022 at 03:03:13PM -0700, Jessica Zhang wrote:
> > > > > Add a custom command line option that allows user to set a custom CRC
> > > > > source. This will allow different drivers to test their own CRC sources
> > > > > instead of just the "auto" option.
> > > > > 
> > > > > Example usage: `./kms_pipe_crc_basic -s intf`
> > > > > 
> > > > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > > > > ---
> > > > >    tests/kms_pipe_crc_basic.c | 28 ++++++++++++++++++++++------
> > > > >    1 file changed, 22 insertions(+), 6 deletions(-)
> > > > 
> > > > While this sounds useful, why only kms_pipe_crc_basic?
> > > > 
> > > > How about checking an environment variable in igt_pipe_crc_new*() and
> > > > overriding the source string based on that, if the parameter is
> > > > "auto"? That would bring this capability to other tests as well.
> > > 
> > > Hey Petri,
> > > 
> > > The intention of this patch is to have *a* way to validate the custom CRC
> > > source. If we implement an environmental variable, we would have to run all
> > > the CRC-related tests manually for validation.
> > 
> > What's the difference though? Between running
> > 
> > ./kms_pipe_crc_basic -s intf
> > 
> > vs.
> > 
> > IGT_OVERRIDE_CRC_METHOD=intf ./kms_pipe_crc_basic
> > 
> > 
> > Or do you mean you want to first validate whether intf works for you
> > at all?
> 
> Yea, we added this patch mainly so we can do basic validation that the intf
> driver code works.

Even so a more generic method for selecting the crc method would be
better, not just for one test. I'm sure this won't be the last time
someone wants to create a new method and needs to test it.

Consider selecting the crc method with an environment variable like
above, or if a command line flag is better for you, add it for all
tests in igt_core.c



-- 
Petri Latvala





> 
> Thanks,
> 
> Jessica Zhang
> 
> > 
> > -- 
> > Petri Latvala
> > 
> > 
> > 
> > > 
> > > Thanks,
> > > 
> > > Jessica Zhang
> > > 
> > > > 
> > > > 
> > > > -- 
> > > > Petri Latvala
> > > > 
> > > > 
> > > > > 
> > > > > diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> > > > > index 0861c46dbd9b..6a593aec4c6c 100644
> > > > > --- a/tests/kms_pipe_crc_basic.c
> > > > > +++ b/tests/kms_pipe_crc_basic.c
> > > > > @@ -36,6 +36,7 @@ typedef struct {
> > > > >    	int debugfs;
> > > > >    	igt_display_t display;
> > > > >    	struct igt_fb fb;
> > > > > +	char *crc_source;
> > > > >    } data_t;
> > > > >    static struct {
> > > > > @@ -104,7 +105,7 @@ static void test_read_crc(data_t *data, enum pipe pipe, unsigned flags)
> > > > >    		if (flags & TEST_NONBLOCK) {
> > > > >    			igt_pipe_crc_t *pipe_crc;
> > > > > -			pipe_crc = igt_pipe_crc_new_nonblock(data->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> > > > > +			pipe_crc = igt_pipe_crc_new_nonblock(data->drm_fd, pipe, data->crc_source);
> > > > >    			igt_wait_for_vblank(data->drm_fd, display->pipes[pipe].crtc_offset);
> > > > >    			igt_pipe_crc_start(pipe_crc);
> > > > > @@ -119,7 +120,7 @@ static void test_read_crc(data_t *data, enum pipe pipe, unsigned flags)
> > > > >    		} else {
> > > > >    			igt_pipe_crc_t *pipe_crc;
> > > > > -			pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> > > > > +			pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe, data->crc_source);
> > > > >    			igt_pipe_crc_start(pipe_crc);
> > > > >    			n_crcs = igt_pipe_crc_get_crcs(pipe_crc, N_CRCS, &crcs);
> > > > > @@ -202,8 +203,7 @@ static void test_compare_crc(data_t *data, enum pipe pipe)
> > > > >    	igt_plane_set_fb(primary, &fb0);
> > > > >    	igt_display_commit(display);
> > > > > -	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> > > > > -				    INTEL_PIPE_CRC_SOURCE_AUTO);
> > > > > +	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe, data->crc_source);
> > > > >    	igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
> > > > >    	/* Flip FB1 with the Primary plane & compare the CRC with ref CRC. */
> > > > > @@ -265,9 +265,25 @@ static void test_disable_crc_after_crtc(data_t *data, enum pipe pipe)
> > > > >    	igt_remove_fb(data->drm_fd, &data->fb);
> > > > >    }
> > > > > -data_t data = {0, };
> > > > > +data_t data = {0, .crc_source = "auto"};
> > > > > -igt_main
> > > > > +static int opt_handler(int opt, int opt_index, void *_data)
> > > > > +{
> > > > > +	switch (opt) {
> > > > > +	case 's':
> > > > > +		data.crc_source = optarg;
> > > > > +		igt_debug("Setting CRC source to %s\n", data.crc_source);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	return IGT_OPT_HANDLER_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +static const char help_str[] =
> > > > > +	"  -s\tPass in a custom source for crc test (default is \"auto\")\n";
> > > > > +
> > > > > +
> > > > > +igt_main_args("s:", NULL, help_str, opt_handler, NULL)
> > > > >    {
> > > > >    	enum pipe pipe;
> > > > > -- 
> > > > > 2.31.0
> > > > >