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

[Jessica Zhang]

On 6/6/2022 1:01 AM, Petri Latvala wrote:
> 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 at 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.

Ah, that's a good point -- will release a v2 to implement this as an 
environmental variable then.

Thanks,

Jessica Zhang

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