Re: [igt-dev] [PATCH i-g-t v2] tests/i915/kms_dsc: add limited flag

["Sharma, Swati2" <swati2.sharma@intel.com>]

On 30-May-23 11:05 PM, Zbigniew Kempczyński wrote:
> On Mon, May 29, 2023 at 10:10:22PM +0530, Swati Sharma wrote:
>> Add limited flag to restrict test execution to one valid pipe-output
>> combination. By default all tests will run on all valid pipe-output
>> combinations.
>>
>> v2: -rename extended to limited (Kamil)
>>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   tests/i915/kms_dsc.c | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/i915/kms_dsc.c b/tests/i915/kms_dsc.c
>> index 3ce28f84..9ddb441d 100644
>> --- a/tests/i915/kms_dsc.c
>> +++ b/tests/i915/kms_dsc.c
>> @@ -51,6 +51,7 @@ typedef struct {
>>   	int input_bpc;
>>   	int disp_ver;
>>   	enum pipe pipe;
>> +	bool limited;
>>   } data_t;
>>   
>>   static int output_format_list[] = {DSC_FORMAT_YCBCR420, DSC_FORMAT_YCBCR444};
>> @@ -250,13 +251,34 @@ static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
>>   
>>   		igt_dynamic_f("pipe-%s-%s%s",  kmstest_pipe_name(data->pipe), data->output->name, name)
>>   			update_display(data, test_type);
>> +
>> +		if (data->limited)
>> +			break;
>>   	}
>>   }
>>   
>> -igt_main
>> +static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>> -	data_t data = {};
> 
> You don't need to introduce static data below, handler has pointer
> to it, and functions are called with &data so keeping this on stack
> is better for me (protects from global access in any place).
> 
>> +	data_t *data = _data;
>> +
>> +	switch (opt) {
>> +	case 'l':
>> +		data->limited = true;
>> +		break;
>> +	default:
>> +		return IGT_OPT_HANDLER_ERROR;
>> +	}
>> +
>> +	return IGT_OPT_HANDLER_SUCCESS;
>> +}
>> +
>> +static const char help_str[] =
>> +	"  --limited\t\tLimit execution to 1 valid pipe-output combo\n";
> 
> I see you're using short option '-l', just add this to line above like:
> 
> "  --limited\t-l\tLimit ..."
> 
>>   
>> +static data_t data = {};
> 
> Keep this on stack. With this nit fixed:
> 
> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

Thanks Zbigniew for the review. With above changes floated v3.

> 
> --
> Zbigniew
> 
>> +
>> +igt_main_args("l", NULL, help_str, opt_handler, &data)
>> +{
>>   	igt_fixture {
>>   		data.drm_fd = drm_open_driver_master(DRIVER_INTEL | DRIVER_XE);
>>   		data.devid = intel_get_drm_devid(data.drm_fd);
>> -- 
>> 2.25.1
>>