[PATCH][V4] lib/igt_kms: Move get_writeback_formats_blob to lib

[Karthik B S]

Hi Alex,

On 6/26/2025 1:54 PM, Kamil Konieczny wrote:
> Hi Alex,
> On 2025-06-25 at 11:41:15 -0600, Alex Hung wrote:
>> From: Harry Wentland <harry.wentland at amd.com>
>>
>> This function can be used by other tests that need writeback. A prefix
>> "igt_" is also added to the function name.
>>
>> Signed-off-by: Alex Hung <alex.hung at amd.com>
>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>> ---
>>   lib/igt_kms.c         | 25 +++++++++++++++++++++++++
>>   lib/igt_kms.h         |  2 ++
>>   tests/kms_writeback.c | 22 ++--------------------
>>   3 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 9ee03e870..d71076f24 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -7472,3 +7472,28 @@ int igt_backlight_write(int value, const char *fname, igt_backlight_context_t *c
>>   
>>   	return 0;
>>   }
>> +
>> +/**
>> + * igt_get_writeback_formats_blob:
>> + * @output: Target output
>> + *
>> + * get supported formats from the writeback connector
>> + *
>> + * Returns: pointer to the writeback formats blob or NULL if not available
>> + */
>> +drmModePropertyBlobRes *igt_get_writeback_formats_blob(igt_output_t *output)
>> +{
>> +	drmModePropertyBlobRes *blob = NULL;
>> +	uint64_t blob_id;
>> +	int ret;
>> +
>> +	ret = kmstest_get_property(output->display->drm_fd,
>> +				   output->config.connector->connector_id,
>> +				   DRM_MODE_OBJECT_CONNECTOR,
>> +				   igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS],
>> +				   NULL, &blob_id, NULL);
>> +	if (ret)
>> +		blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id);
>> +
>> +	return blob;
>> +}
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index a85fee515..09d7d4414 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -1281,4 +1281,6 @@ void igt_set_link_params(int drm_fd, igt_output_t *output,
>>   int igt_backlight_read(int *result, const char *fname, igt_backlight_context_t *context);
>>   int igt_backlight_write(int value, const char *fname, igt_backlight_context_t *context);
>>   
>> +drmModePropertyBlobRes *igt_get_writeback_formats_blob(igt_output_t *output);
>> +
>>   #endif /* __IGT_KMS_H__ */
>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>> index 8765fda34..b4c3b1cd2 100644
>> --- a/tests/kms_writeback.c
>> +++ b/tests/kms_writeback.c
>> @@ -92,25 +92,6 @@ enum {
>>   	XRGB2101010 = 1 << 1,
>>   };
>>   
>> -static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
>> -{
>> -	drmModePropertyBlobRes *blob = NULL;
>> -	uint64_t blob_id;
>> -	int ret;
>> -
>> -	ret = kmstest_get_property(output->display->drm_fd,
>> -				   output->config.connector->connector_id,
>> -				   DRM_MODE_OBJECT_CONNECTOR,
>> -				   igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS],
>> -				   NULL, &blob_id, NULL);
>> -	if (ret)
>> -		blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id);
>> -
>> -	igt_assert(blob);
>> -
>> -	return blob;
>> -}
>> -
>>   static bool check_writeback_config(igt_display_t *display, igt_output_t *output,
>>   				    drmModeModeInfo override_mode)
>>   {
>> @@ -621,7 +602,8 @@ igt_main_args("b:c:f:dl", long_options, help_str, opt_handler, NULL)
>>   		const char *valid_chars;
>>   
>>   		igt_skip_on(data.dump_check || data.list_modes);
>> -		formats_blob = get_writeback_formats_blob(output);
>> +		formats_blob = igt_get_writeback_formats_blob(output);
>> +		igt_require_f(formats_blob, "No writeback pixel formats\n");
> Now imho it looks good but I am not KMS dev so I am not sure
> if not having pixel formats is a bug or not, e.g. if we need
> igt_assert_f here or igt_require_f is good enough.
>
> Khartik or Swati or Juha-Pekka, could you help here?

I would keep it as an igt_assert_f itself, which would also retain the 
existing test behavior.

with this updated,

Reviewed-by: Karthik B S <karthik.b.s at intel.com>

>
> +cc Karthik B S <karthik.b.s at intel.com>
> Cc: Juha-Pekka Heikkila <juha-pekka.heikkila at intel.com>
> Cc: Swati Sharma <swati2.sharma at intel.com>
>
> So moving function to lib looks ok, for that part
>
> Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>
>>   		valid_chars = "01234568 ABCGNRUVXY";
>>   
>>   		/*
>> -- 
>> 2.43.0
>>