Re: [igt-dev] [Patch][i-g-t 1/2] lib/igt_kms: Add support for detecting connector events

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

Hi Ram,

Thanks for the review comments. Please find my response inline:

On 6/16/2020 7:42 PM, Ramalingam C wrote:
> On 2020-06-16 at 16:28:11 +0530, Ankit Nautiyal wrote:
>> Currently, the event_detect function checks the property val for
>> "HOTPLUG" and "LEASE" both of which are set to '1' when these events
>> are sent.
>>
>> This cannot be used for detecting connector events such as HDCP event
>> as connector events are sent along with property to signify which
>> property of which connector has changed. Connector ID and property id
>> are provided along with "CONNECTOR" and "PROPERTY" as udev
>> property-value pairs. Eg. for HDCP, the connector id of the connector
>> whose hdcp status changed, and the property id of the
>> ‘CONTENT_PROTECTION’ property are sent with uevent.
>>
>> This patch modifies the event_detect function to check multiple
>> properties with different expected values. It also adds support to
>> detect connector event for a given pair of connector and property ids.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   lib/igt_kms.c | 61 ++++++++++++++++++++++++++++++++++++++-------------
>>   lib/igt_kms.h |  2 ++
>>   2 files changed, 48 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 54de45e5..7177231e 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -4164,32 +4164,57 @@ struct udev_monitor *igt_watch_hotplug(void)
>>   }
>>   
>>   static bool event_detected(struct udev_monitor *mon, int timeout_secs,
>> -			   const char *property)
>> +			   const char **property, int *expected_val, int num_props)
>>   {
>>   	struct udev_device *dev;
>> -	const char *hotplug_val;
>> +	const char *prop_val_str;
>>   	struct pollfd fd = {
>>   		.fd = udev_monitor_get_fd(mon),
>>   		.events = POLLIN
>>   	};
>> -	bool hotplug_received = false;
>> +	bool event_received = false;
>> +	int i;
>>   
>> -	/* Go through all of the events pending on the udev monitor. Once we
>> -	 * receive a hotplug, we continue going through the rest of the events
>> -	 * so that redundant hotplug events don't change the results of future
>> -	 * checks
>> +	/* Go through all of the events pending on the udev monitor.
>> +	 * Match the given set of properties and their values to
>> +	 * the expected values.
>>   	 */
>> -	while (!hotplug_received && poll(&fd, 1, timeout_secs * 1000)) {
>> +	while (!event_received && poll(&fd, 1, timeout_secs * 1000)) {
>>   		dev = udev_monitor_receive_device(mon);
>> -
>> -		hotplug_val = udev_device_get_property_value(dev, property);
>> -		if (hotplug_val && atoi(hotplug_val) == 1)
>> -			hotplug_received = true;
>> +		for (i = 0; i < num_props; i++) {
>> +			event_received = true;
> Not needed.
>> +			prop_val_str = udev_device_get_property_value(dev, property[i]);
>> +			if (!prop_val_str || atoi(prop_val_str) != expected_val[i]) {
>> +				event_received = false;
> Not needed.
>> +				break;
>> +			}
>> +		}
> 		if (i == num_props)
> 			event_received = true;

Agreed, this is much elegant. Will use this in the next version.

>>   
>>   		udev_device_unref(dev);
>>   	}
>>   
>> -	return hotplug_received;
>> +	return event_received;
>> +}
>> +
>> +/**
>> + * igt_conn_event_detected:
>> + * @mon: A udev monitor initialized with #igt_watch_hotplug
>> + * @conn_id: Connector id of the Connector for which the property change is
>> + * expected.
>> + * @prop_id: Property id for which the change is expected.
>> + * @timeout_secs: How long to wait for a connector event to occur.
>> + *
>> + * Assert that a connector event is received for a given connector and property.
>> + *
>> + * Returns: true if the connector event was received, false if we timed out
>> + */
>> +bool igt_conn_event_detected(struct udev_monitor *mon, uint32_t conn_id,
>> +			     uint32_t prop_id, int timeout_secs)
>> +{
>> +	const char *props[2] = {"CONNECTOR", "PROPERTY"};
>> +	int expected_val[2] = {conn_id, prop_id};
>> +
>> +	return event_detected(mon, timeout_secs, props, expected_val, 2);
>>   }
>>   
>>   /**
>> @@ -4203,7 +4228,10 @@ static bool event_detected(struct udev_monitor *mon, int timeout_secs,
>>    */
>>   bool igt_hotplug_detected(struct udev_monitor *mon, int timeout_secs)
>>   {
>> -	return event_detected(mon, timeout_secs, "HOTPLUG");
>> +	const char *props[1] = {"HOTPLUG"};
>> +	int expected_val = 1;
>> +
>> +	return event_detected(mon, timeout_secs, props, &expected_val, 1);
>>   }
>>   
>>   /**
>> @@ -4217,7 +4245,10 @@ bool igt_hotplug_detected(struct udev_monitor *mon, int timeout_secs)
>>    */
>>   bool igt_lease_change_detected(struct udev_monitor *mon, int timeout_secs)
>>   {
>> -	return event_detected(mon, timeout_secs, "LEASE");
>> +	const char *props[1] = {"LEASE"};
>> +	int expected_val = 1;
>> +
>> +	return event_detected(mon, timeout_secs, props, &expected_val, 1);
>>   }
>>   
>>   /**
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index cd3fdbc0..27f1f729 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -777,6 +777,8 @@ bool igt_hotplug_detected(struct udev_monitor *mon,
>>   			  int timeout_secs);
>>   bool igt_lease_change_detected(struct udev_monitor *mon,
>>   			       int timeout_secs);
>> +bool igt_conn_event_detected(struct udev_monitor *mon, uint32_t conn_id,
>> +			     uint32_t prop_id, int timeout_msecs);
>>   void igt_flush_hotplugs(struct udev_monitor *mon);
> We need to have generich flush too like igt_(watch/flush/cleanup)_udev_event()

I am open to generic names, more about this discussed in the next patch 2/2.

Regards,
Ankit

> -Ram
>>   void igt_cleanup_hotplug(struct udev_monitor *mon);
>>   
>> -- 
>> 2.17.1
>>

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev