Re: [i-g-t,3/3] lib/kms: Generalize forced_connectors[] to handle arbitrary attributes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 11, 2024 at 01:17:19PM +0530, Joshi, Kunal1 wrote:
> Hello Ville,
> 
> On 4/10/2024 8:32 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä<ville.syrjala@linux.intel.com>
> >
> > We may want to poke at various other connector attributes
> > via sysfs/debugfs. Generalize the existing the force_connectors
> > mechamisn to handle arbitrary attributes.
> >
> > Cc: Kunal Joshi<kunal1.joshi@intel.com>
> > Signed-off-by: Ville Syrjälä<ville.syrjala@linux.intel.com>
> > ---
> >   lib/igt_kms.c | 131 ++++++++++++++++++++++++++++++++------------------
> >   1 file changed, 85 insertions(+), 46 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 19bb4ac66ece..5b23fada94d9 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -91,14 +91,18 @@
> >   #define MAX_EDID 2
> >   #define DISPLAY_TILE_BLOCK 0x12
> >   
> > -struct igt_forced_connector {
> > +typedef bool (*igt_connector_attr_set)(int dir, const char *attr, const char *value);
> > +
> > +struct igt_connector_attr {
> >   	uint32_t connector_type;
> >   	uint32_t connector_type_id;
> >   	int idx;
> >   	int dir;
> > +	igt_connector_attr_set set;
> > +	const char *attr, *value, *reset_value;
> >   };
> >   
> > -static struct igt_forced_connector forced_connectors[MAX_CONNECTORS + 1];
> > +static struct igt_connector_attr connector_attrs[MAX_CONNECTORS + 1];
> >   
> >   /**
> >    * igt_kms_get_base_edid:
> > @@ -1495,39 +1499,80 @@ int igt_connector_sysfs_open(int drm_fd,
> >   	return conn_dir;
> >   }
> >   
> > -static bool connector_is_forced(int idx, drmModeConnector *connector)
> > +static struct igt_connector_attr *connector_attr_find(int idx, drmModeConnector *connector,
> > +						      igt_connector_attr_set set,
> > +						      const char *attr)
> >   {
> >   	igt_assert(connector->connector_type != 0);
> >   
> > -	for (int i = 0; forced_connectors[i].connector_type; i++) {
> > -		struct igt_forced_connector *c = &forced_connectors[i];
> > +	for (int i = 0; connector_attrs[i].connector_type; i++) {
> > +		struct igt_connector_attr *c = &connector_attrs[i];
> >   
> >   		if (c->idx == idx &&
> >   		    c->connector_type == connector->connector_type &&
> > -		    c->connector_type_id == connector->connector_type_id)
> > -			return true;
> > +		    c->connector_type_id == connector->connector_type_id &&
> > +		    c->set == set && !strcmp(c->attr, attr))
> > +			return c;
> >   	}
> >   
> > -	return false;
> > +	return NULL;
> >   }
> >   
> > -static struct igt_forced_connector *forced_connector_alloc(void)
> > +static struct igt_connector_attr *connector_attr_alloc(int idx, drmModeConnector *connector,
> > +						       int dir, igt_connector_attr_set set,
> > +						       const char *attr, const char *reset_value)
> >   {
> > -	int i;
> > +	struct igt_connector_attr *c = NULL;
> >   
> > -	for (i = 0; forced_connectors[i].connector_type; i++)
> > -		;
> > +	for (int i = 0; connector_attrs[i].connector_type; i++) {
> > +		c = &connector_attrs[i];
> > +		break;
> > +	}
> > +
> > +	c->idx = idx;
> > +	c->connector_type = connector->connector_type;
> > +	c->connector_type_id = connector->connector_type_id;
> > +
> > +	c->dir = dir;
> > +	c->set = set;
> > +	c->attr = attr;
> > +	c->reset_value = reset_value;
> 
> c can be null if we don't find any valid connector_attrs

c==NULL would be a bug.

> 
> Thanks and Regards Kunal Joshi
> 
> > +
> > +	return c;
> > +}
> > +
> > +static void connector_attr_free(struct igt_connector_attr *c)
> > +{
> > +	memset(c, 0, sizeof(*c));
> > +}
> > +
> > +static bool connector_attr_set(int idx, drmModeConnector *connector,
> > +			       int dir, igt_connector_attr_set set,
> > +			       const char *attr, const char *value,
> > +			       const char *reset_value)
> > +{
> > +	struct igt_connector_attr *c;
> > +
> > +	c = connector_attr_find(idx, connector, set, attr);
> > +	if (!c)
> > +		c = connector_attr_alloc(idx, connector, dir, set,
> > +					 attr, reset_value);
> > +
> > +	if (!c->set(c->dir, c->attr, c->value)) {
> > +		connector_attr_free(c);
> > +		return false;
> > +	}
> >   
> > -	igt_assert_lt(i, ARRAY_SIZE(forced_connectors));
> > +	c->value = value;
> >   
> > -	return &forced_connectors[i];
> > +	return true;
> >   }
> >   
> > -static bool force_connector(int drm_fd,
> > -			    drmModeConnector *connector,
> > -			    const char *value)
> > +static bool connector_attr_set_sysfs(int drm_fd,
> > +				     drmModeConnector *connector,
> > +				     const char *attr, const char *value,
> > +				     const char *reset_value)
> >   {
> > -	struct igt_forced_connector *c;
> >   	char name[80];
> >   	int idx, dir;
> >   
> > @@ -1543,45 +1588,39 @@ static bool force_connector(int drm_fd,
> >   	if (dir < 0)
> >   		return false;
> >   
> > -	if (!igt_sysfs_set(dir, "status", value)) {
> > -		close(dir);
> > +	if (!connector_attr_set(idx, connector, dir,
> > +				igt_sysfs_set, attr, value, reset_value))
> >   		return false;
> > -	}
> >   
> > -	igt_debug("Connector %s is now forced %s\n", name, value);
> > -
> > -	/* already tracked? */
> > -	if (connector_is_forced(idx, connector)) {
> > -		close(dir);
> > -		return true;
> > -	}
> > -
> > -	c = forced_connector_alloc();
> > -
> > -	c->idx = idx;
> > -	c->connector_type = connector->connector_type;
> > -	c->connector_type_id = connector->connector_type_id;
> > -	c->dir = dir;
> > +	igt_debug("Connector %s/%s is now %s\n", name, attr, value);
> >   
> >   	return true;
> >   }
> >   
> > -static void dump_forced_connectors(void)
> > +static void dump_connector_attrs(void)
> >   {
> >   	char name[80];
> >   
> > -	igt_debug("Current forced connectors:\n");
> > +	igt_debug("Current connector attrs:\n");
> >   
> > -	for (int i = 0; forced_connectors[i].connector_type; i++) {
> > -		struct igt_forced_connector *c = &forced_connectors[i];
> > +	for (int i = 0; connector_attrs[i].connector_type; i++) {
> > +		struct igt_connector_attr *c = &connector_attrs[i];
> >   
> >   		kmstest_connector_dirname(c->idx, c->connector_type,
> >   					  c->connector_type_id,
> >   					  name, sizeof(name));
> > -		igt_debug("\t%s\n", name);
> > +		igt_debug("\t%s/%s: %s\n", name, c->attr, c->value);
> >   	}
> >   }
> >   
> > +static bool force_connector(int drm_fd,
> > +			    drmModeConnector *connector,
> > +			    const char *value)
> > +{
> > +	return connector_attr_set_sysfs(drm_fd, connector,
> > +					"status", value, "detect");
> > +}
> > +
> >   /**
> >    * kmstest_force_connector:
> >    * @fd: drm file descriptor
> > @@ -1626,7 +1665,7 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector *connector,
> >   	if (!force_connector(drm_fd, connector, value))
> >   		return false;
> >   
> > -	dump_forced_connectors();
> > +	dump_connector_attrs();
> >   
> >   	igt_install_exit_handler(reset_connectors_at_exit);
> >   
> > @@ -2619,7 +2658,7 @@ static void igt_handle_spurious_hpd(igt_display_t *display)
> >   			 conn->connector_type_id);
> >   	}
> >   
> > -	dump_forced_connectors();
> > +	dump_connector_attrs();
> >   }
> >   
> >   /**
> > @@ -5303,12 +5342,12 @@ void igt_enable_connectors(int drm_fd)
> >    */
> >   void igt_reset_connectors(void)
> >   {
> > -	/* reset the connectors stored in forced_connectors, avoiding any
> > +	/* reset the connectors stored in connector_attrs, avoiding any
> >   	 * functions that are not safe to call in signal handlers */
> > -	for (int i = 0; i < forced_connectors[i].connector_type; i++) {
> > -		struct igt_forced_connector *c = &forced_connectors[i];
> > +	for (int i = 0; i < connector_attrs[i].connector_type; i++) {
> > +		struct igt_connector_attr *c = &connector_attrs[i];
> >   
> > -		igt_sysfs_set(c->dir, "status",  "detect");
> > +		c->set(c->dir, c->attr, c->reset_value);
> >   	}
> >   }
> >   

-- 
Ville Syrjälä
Intel