Re: [i-g-t V2 1/5] lib/igt_kms: Add infra to handle crtc arbitrary attributes

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

Hi Kamil,

On 15-11-2024 10:50 pm, Kamil Konieczny wrote:
> Hi Bhanuprakash,
> On 2024-09-24 at 19:56:34 +0530, Bhanuprakash Modem wrote:
>> We may want to poke at various other crtc attributes
>> via sysfs/debugfs. Add a mechamisn to handle crtc
>> arbitrary attributes.
>>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> ---
>>   lib/igt_kms.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_kms.h |   3 +
>>   2 files changed, 153 insertions(+)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index b40470c02..c22cdb602 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -92,6 +92,7 @@
>>   #define DISPLAY_TILE_BLOCK 0x12
>>   
>>   typedef bool (*igt_connector_attr_set)(int dir, const char *attr, const char *value);
>> +typedef bool (*igt_crtc_attr_set)(int dir, const char *attr, const char *value);
>>   
>>   struct igt_connector_attr {
>>   	uint32_t connector_type;
>> @@ -102,7 +103,16 @@ struct igt_connector_attr {
>>   	const char *attr, *value, *reset_value;
>>   };
>>   
>> +struct igt_crtc_attr {
>> +	int pipe;
>> +	int idx;
>> +	int dir;
>> +	igt_crtc_attr_set set;
>> +	const char *attr, *value, *reset_value;
>> +};
>> +
>>   static struct igt_connector_attr connector_attrs[MAX_CONNECTORS];
>> +static struct igt_crtc_attr crtc_attrs[IGT_MAX_PIPES];
>>   
>>   /**
>>    * igt_kms_get_base_edid:
>> @@ -1802,6 +1812,125 @@ void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
>>   	igt_assert(ret != -1);
>>   }
>>   
>> +static struct igt_crtc_attr *crtc_attr_find(int idx, enum pipe pipe,
>> +					    igt_crtc_attr_set set,
>> +					    const char *attr)
>> +{
>> +	igt_assert(pipe != PIPE_NONE || pipe >= IGT_MAX_PIPES);
> 
> Why not return NULL for PIPE_NONE?

If we return NULL from here, crtc_attr_set() will allocate some memory 
which is not an valid case. So we must need this check either here or in 
crtc_attr_set().

> Also imho better:
> 
> 	igt_assert(pipe >= 0 && pipe < ARRAY_SIZE());

Sure, will do that in next rev.

> 
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(crtc_attrs); i++) {
>> +		struct igt_crtc_attr *c = &crtc_attrs[i];
>> +
>> +		if (c->idx == idx &&
>> +		    c->pipe == pipe &&
>> +		    c->set == set && !strcmp(c->attr, attr))
>> +			return c;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct igt_crtc_attr *crtc_attr_find_free(void)
>> +{
>> +	for (int i = 0; i < ARRAY_SIZE(crtc_attrs); i++) {
>> +		struct igt_crtc_attr *c = &crtc_attrs[i];
>> +
>> +		if (!c->attr)
>> +			return c;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct igt_crtc_attr *crtc_attr_alloc(int idx, enum pipe pipe,
>> +					     int dir, igt_crtc_attr_set set,
>> +					     const char *attr, const char *reset_value)
>> +{
>> +	struct igt_crtc_attr *c = crtc_attr_find_free();
> 
> c could be NULL here, if find_free() failed.

I'll fix this in next rev.

- Bhanu

> 
> Regards,
> Kamil
> 
>> +
>> +	c->idx = idx;
>> +	c->pipe = pipe;
>> +
>> +	c->dir = dir;
>> +	c->set = set;
>> +	c->attr = attr;
>> +	c->reset_value = reset_value;
>> +
>> +	return c;
>> +}
>> +
>> +static void crtc_attr_free(struct igt_crtc_attr *c)
>> +{
>> +	memset(c, 0, sizeof(*c));
>> +}
>> +
>> +static bool crtc_attr_set(int idx, enum pipe pipe,
>> +			  int dir, igt_crtc_attr_set set,
>> +			  const char *attr, const char *value,
>> +			  const char *reset_value)
>> +{
>> +	struct igt_crtc_attr *c;
>> +
>> +	c = crtc_attr_find(idx, pipe, set, attr);
>> +	if (!c)
>> +		c = crtc_attr_alloc(idx, pipe, dir, set, attr, reset_value);
>> +
>> +	c->value = value;
>> +
>> +	if (!c->set(c->dir, c->attr, c->value)) {
>> +		crtc_attr_free(c);
>> +		return false;
>> +	}
>> +
>> +	if (!strcmp(c->value, c->reset_value))
>> +		crtc_attr_free(c);
>> +
>> +	return true;
>> +}
>> +
>> +static bool crtc_attr_set_debugfs(int drm_fd, enum pipe pipe,
>> +				  const char *attr, const char *value,
>> +				  const char *reset_value)
>> +{
>> +	int idx, dir;
>> +
>> +	idx = igt_device_get_card_index(drm_fd);
>> +	if (idx < 0 || idx > 63)
>> +		return false;
>> +
>> +	dir = igt_debugfs_pipe_dir(drm_fd, pipe, O_DIRECTORY);
>> +	if (dir < 0)
>> +		return false;
>> +
>> +	if (!crtc_attr_set(idx, pipe, dir,
>> +			   igt_sysfs_set, attr,
>> +			   value, reset_value))
>> +		return false;
>> +
>> +	igt_info("Crtc-%d/%s is now %s\n", pipe, attr, value);
>> +
>> +	return true;
>> +}
>> +
>> +/**
>> + * kmstest_set_connector_dpms:
>> + *
>> + * Dump all attr state on all crtcs.
>> + */
>> +void dump_crtc_attrs(void)
>> +{
>> +	igt_debug("Current crtc attrs:\n");
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(crtc_attrs); i++) {
>> +		struct igt_crtc_attr *c = &crtc_attrs[i];
>> +
>> +		if (!c->attr)
>> +			continue;
>> +
>> +		igt_debug("\tcrtc-%d/%s: %s\n", c->pipe, c->attr, c->value);
>> +	}
>> +}
>> +
>>   /**
>>    * sort_drm_modes_by_clk_dsc:
>>    * @a: first element
>> @@ -5459,6 +5588,27 @@ void igt_reset_connectors(void)
>>   	}
>>   }
>>   
>> +/**
>> + * igt_reset_crtcs:
>> + *
>> + * Remove any forced state from the crtcs.
>> + */
>> +void igt_reset_crtcs(void)
>> +{
>> +	/*
>> +	 * Reset the crtcs stored in crtc_attrs, avoiding any
>> +	 * functions that are not safe to call in signal handlers
>> +	 */
>> +	for (int i = 0; i < ARRAY_SIZE(crtc_attrs); i++) {
>> +		struct igt_crtc_attr *c = &crtc_attrs[i];
>> +
>> +		if (!c->attr)
>> +			continue;
>> +
>> +		c->set(c->dir, c->attr, c->reset_value);
>> +	}
>> +}
>> +
>>   /**
>>    * igt_watch_uevents:
>>    *
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 25ba50916..97efbefa5 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -1085,6 +1085,9 @@ void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>>   void igt_enable_connectors(int drm_fd);
>>   void igt_reset_connectors(void);
>>   
>> +void igt_reset_crtcs(void);
>> +void dump_crtc_attrs(void);
>> +
>>   uint32_t kmstest_get_vbl_flag(int crtc_offset);
>>   
>>   const struct edid *igt_kms_get_base_edid(void);
>> -- 
>> 2.43.0
>>