Re: [PATCH i-g-t v5 1/2] lib/igt_kms: Add support to retrieve async modifiers and formats

["Reddy Guddati, Santhosh" <santhosh.reddy.guddati@intel.com>]

On 17-03-2025 14:26, Borah, Chaitanya Kumar wrote:
> 
> 
>> -----Original Message-----
>> From: Reddy Guddati, Santhosh <santhosh.reddy.guddati@intel.com>
>> Sent: Tuesday, March 11, 2025 2:24 PM
>> To: igt-dev@lists.freedesktop.org
>> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; B S, Karthik
>> <karthik.b.s@intel.com>; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@intel.com>; Reddy Guddati, Santhosh
>> <santhosh.reddy.guddati@intel.com>
>> Subject: [PATCH i-g-t v5 1/2] lib/igt_kms: Add support to retrieve async
>> modifiers and formats
>>
>> Parse "IN_FORMATS_ASYNC" plane property to identify supported format
>> modifier pairs for async flips
>>
>> V2: Add new fields async_formats and reset idx.
>>
>> V3: Improve commit message , remove unused declaration (Chaitanya)
>>      Introduce structure to hold a format modifier and its
>>      associated format list.
>>
>> V4: Implement format+modifier tuples for async similar to IN_FORMATS
>>      (Chaitanya).
>>
>> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>> ---
>>   lib/igt_kms.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>   lib/igt_kms.h |  7 ++++++-
>>   2 files changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index cc3bb3ae7..dfb23c45e 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -701,6 +701,7 @@ const char * const
>> igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>>   	[IGT_PLANE_FB_DAMAGE_CLIPS] = "FB_DAMAGE_CLIPS",
>>   	[IGT_PLANE_SCALING_FILTER] = "SCALING_FILTER",
>>   	[IGT_PLANE_SIZE_HINTS] = "SIZE_HINTS",
>> +	[IGT_PLANE_IN_FORMATS_ASYNC] = "IN_FORMATS_ASYNC",
>>   };
>>
>>   const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = { @@ -
>> 5758,11 +5759,14 @@ static int igt_count_plane_format_mod(const struct
>> drm_format_modifier_blob *blo
>>
>>   static void igt_fill_plane_format_mod(igt_display_t *display, igt_plane_t
>> *plane)  {
>> -	const struct drm_format_modifier_blob *blob_data;
>> +	const struct drm_format_modifier_blob *blob_data,
>> *async_blob_data;
>> +	const struct drm_format_modifier *async_modifiers;
>>   	drmModePropertyBlobPtr blob;
>>   	uint64_t blob_id;
>>   	int idx = 0;
>>   	int count;
>> +	int async_count;
>> +	const uint32_t *async_formats;
>>
> 
> I don't see a reason why we can't re-use the blob_data and count local variables.
> 
> But that got me thinking. Are we leaking the blob?
> 
> I don't see a drmModeFreePropertyBlob() call made after drmModeGetPropertyBlob() for both sync and async.
> 
> Otherwise the change looks good.

Re used the local variables for the async formats and yes, there was no 
drmModeFreePropertyBlob() for sync formats as well. Added this for both 
sync and async cases.

Thanks,
Santhosh>
> Regards
> 
> Chaitanya
> 
>>   	if (!igt_plane_has_prop(plane, IGT_PLANE_IN_FORMATS)) {
>>   		drmModePlanePtr p = plane->drm_plane; @@ -5822,6
>> +5826,46 @@ static void igt_fill_plane_format_mod(igt_display_t *display,
>> igt_plane_t *plane
>>   	}
>>
>>   	igt_assert_eq(idx, plane->format_mod_count);
>> +
>> +	if (!igt_plane_has_prop(plane, IGT_PLANE_IN_FORMATS_ASYNC))
>> +		return;
>> +
>> +	blob_id = igt_plane_get_prop(plane,
>> IGT_PLANE_IN_FORMATS_ASYNC);
>> +	blob = drmModeGetPropertyBlob(display->drm_fd, blob_id);
>> +
>> +	if (!blob)
>> +		return;
>> +
>> +	async_blob_data = (const struct drm_format_modifier_blob *)blob-
>>> data;
>> +	async_count = igt_count_plane_format_mod(async_blob_data);
>> +
>> +	if (!async_count)
>> +		return;
>> +
>> +	plane->async_format_mod_count = async_count;
>> +	plane->async_modifiers = calloc(async_count, sizeof(plane-
>>> async_modifiers[0]));
>> +	igt_assert(plane->async_modifiers);
>> +
>> +	plane->async_formats = calloc(async_count, sizeof(plane-
>>> async_formats[0]));
>> +	igt_assert(plane->async_formats);
>> +
>> +	idx = 0;
>> +	for (int i = 0; i < async_blob_data->count_modifiers; i++) {
>> +		for (int j = 0; j < 64; j++) {
>> +			async_modifiers = modifiers_ptr(async_blob_data);
>> +			async_formats = formats_ptr(async_blob_data);
>> +
>> +			if (!(async_modifiers[i].formats & (1ULL << j)))
>> +				continue;
>> +
>> +			plane->async_formats[idx] =
>> async_formats[async_modifiers[i].offset + j];
>> +			plane->async_modifiers[idx] =
>> async_modifiers[i].modifier;
>> +			idx++;
>> +			igt_assert_lte(idx, plane->async_format_mod_count);
>> +		}
>> +	}
>> +
>> +	igt_assert_eq(idx, plane->async_format_mod_count);
>>   }
>>
>>   /**
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 27b545f52..78d6a7ee4 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -365,7 +365,8 @@ enum igt_atomic_plane_properties {
>>          IGT_PLANE_HOTSPOT_X,
>>          IGT_PLANE_HOTSPOT_Y,
>>          IGT_PLANE_SIZE_HINTS,
>> -       IGT_NUM_PLANE_PROPS
>> +       IGT_PLANE_IN_FORMATS_ASYNC,
>> +       IGT_NUM_PLANE_PROPS,
>>   };
>>
>>   /**
>> @@ -438,6 +439,10 @@ typedef struct igt_plane {
>>   	uint64_t *modifiers;
>>   	uint32_t *formats;
>>   	int format_mod_count;
>> +
>> +	uint64_t *async_modifiers;
>> +	uint32_t *async_formats;
>> +	int async_format_mod_count;
>>   } igt_plane_t;
>>
>>   /*
>> --
>> 2.34.1
>