Re: [PATCH i-g-t 3/4] lib/igt_kms: add helper to enable/disable force joiner

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

On 3/10/2024 7:57 PM, Kunal Joshi wrote:
> add helpers to check whether force joiner debugfs exists
> and to enable/disable force joiner for a specific connector.
>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> ---
>   lib/igt_kms.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_kms.h |  2 ++
>   2 files changed, 61 insertions(+)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 63c8045c7..9d0cbd329 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -6168,6 +6168,65 @@ bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>   	return found;
>   }
>   
> +/**
> + * Checks if the force big joiner is enabled for a specific connector.
> + *
> + * @drmfd The file descriptor of the DRM device.
> + * @connector_name The name of the connector.

I think we dont need documentation for this, as this is straight forward 
and just static function which is called from one place.

> + * Returns:
> + *  true if status equals enable, false otherwise.
> + */
> +static bool igt_check_force_bigjoiner_status(int drmfd, char *connector_name, bool enable)
> +{
> +	char buf[512];
> +	int debugfs_fd, ret;
> +
> +	igt_assert_f(connector_name, "Connector name cannot be NULL\n");
> +	debugfs_fd = igt_debugfs_connector_dir(drmfd, connector_name, O_RDONLY);
> +	igt_assert_f(debugfs_fd >= 0, "Could not open debugfs for connector %s\n", connector_name);
> +	ret = igt_debugfs_simple_read(debugfs_fd, "i915_bigjoiner_force_enable", buf, sizeof(buf));
> +	close(debugfs_fd);
> +	igt_assert_f(ret > 0, "Could not read i915_bigjoiner_force_enable for connector %s\n", connector_name);
> +	return enable ? strstr(buf, "Bigjoiner enable: 1") :
> +					strstr(buf, "Bigjoiner enable: 0");
> +}
> +
> +bool has_force_joiner_debugfs(int drmfd, igt_output_t *output)

Since this helper is expected to be used in other tests, it would be 
good to have documentation for this.

Also, imho, it would be better to use igt_* for helpers, though we seem 
to be not following this strictly.


> +{
> +	char buf[512];
> +	int debugfs_fd, ret;
> +
> +	igt_assert_f(output->name, "Connector name cannot be NULL\n");
> +	debugfs_fd = igt_debugfs_connector_dir(drmfd, output->name, O_RDONLY);
> +	igt_assert_f(debugfs_fd >= 0, "Could not open debugfs for connector %s\n", output->name);

I think we should not assert here, this will fail for platforms that do 
not support bigjoiner. Perhaps returning false will be sufficient.


Regards,

Ankit

> +	ret = igt_debugfs_simple_read(debugfs_fd, "i915_bigjoiner_force_enable", buf, sizeof(buf));
> +	close(debugfs_fd);
> +	return ret >= 0;
> +}
> +
> +/**
> + * Forces the enable/disable state of big joiner for a specific connector.
> + *
> + * @drmfd The file descriptor of the DRM device.
> + * @connector_name The name of the connector.
> + * @enable The desired state of big joiner (true for enable, false for disable).
> + * Returns:
> + *  true on success, false otherwise.
> + */
> +bool igt_force_bigjoiner_enable(int drmfd, char *connector_name, bool enable)
> +{
> +	int debugfs_fd, ret;
> +
> +	igt_assert_f(connector_name, "Connector name cannot be NULL\n");
> +	debugfs_fd = igt_debugfs_connector_dir(drmfd, connector_name, O_DIRECTORY);
> +	igt_assert_f(debugfs_fd >= 0, "Could not open debugfs for connector %s\n", connector_name);
> +	ret = igt_sysfs_write(debugfs_fd, "i915_bigjoiner_force_enable", enable ? "1" : "0", 1);
> +	close(debugfs_fd);
> +	igt_assert_f(ret > 0, "Could not write i915_bigjoiner_force_enable for connector %s\n", connector_name);
> +
> +	return igt_check_force_bigjoiner_status(drmfd, connector_name, enable);
> +}
> +
>   /**
>    * igt_check_bigjoiner_support:
>    * @display: a pointer to an #igt_display_t structure
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index bab8487d3..f13b7fd53 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -1214,6 +1214,8 @@ int igt_get_max_dotclock(int fd);
>   bool igt_bigjoiner_possible(drmModeModeInfo *mode, int max_dotclock);
>   bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>   			  int max_dotclock);
> +bool has_force_joiner_debugfs(int drmfd, igt_output_t *output);
> +bool igt_force_bigjoiner_enable(int drmfd, char *connector_name, bool enable);
>   bool igt_check_bigjoiner_support(igt_display_t *display);
>   bool igt_parse_mode_string(const char *mode_string, drmModeModeInfo *mode);
>   bool intel_pipe_output_combo_valid(igt_display_t *display);