[PATCH i-g-t] tests/intel/kms_pm_dc: Modified the deep-pkgc subtest. Changes: 1. Added checks to avoid connector failures. 2. Added vblank wait to sync with frame time.

[Kamil Konieczny]

Hi Naladala,
On 2024-07-03 at 22:43:39 +0530, Naladala Ramanaidu wrote:

your subject is too long:

[PATCH i-g-t] tests/intel/kms_pm_dc: Modified the deep-pkgc
 subtest. Changes: 1. Added checks to avoid connector failures. 2. Added
 vblank wait to sync with frame time.

it should be shorter, for example:

[PATCH i-g-t] tests/intel/kms_pm_dc: Fixed deep-pkgc subtest

How to write a git commit message: https://cbea.ms/git-commit/
other usfull links from CONTRIBUTING.md:

https://kernelnewbies.org/PatchPhilosophy
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/submit-checklist.html

btw from your subject it looks like you are doing two changes,
consider splitting this into two pathes:

[PATCH i-g-t 1/2] tests/intel/kms_pm_dc: avoid connector failures in deep-pkgc
[PATCH i-g-t 2/2] tests/intel/kms_pm_dc: sync with frame time in deep-pkgc

Add cc here before s-o-b:

Cc: Jeevan B <jeevan.b at intel.com>
> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala at intel.com>
> ---
>  tests/intel/kms_pm_dc.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
> index e1318bfa6..b92478d50 100644
> --- a/tests/intel/kms_pm_dc.c
> +++ b/tests/intel/kms_pm_dc.c
> @@ -592,7 +592,7 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
>  static void test_deep_pkgc_state(data_t *data)
>  {
>  	unsigned int pre_val = 0, cur_val = 0;
> -	time_t start = time(NULL), duration = 2, delay;
> +	time_t start = time(NULL), duration = 4, vb_delay;
>  	enum pipe pipe;
>  	bool pkgc_flag = false;
>  	bool vrr_supported = false, flip = true;
> @@ -602,6 +602,12 @@ static void test_deep_pkgc_state(data_t *data)
>  	igt_output_t *output = NULL;
>  
>  	for_each_pipe_with_valid_output(display, pipe, output) {
> +
> +		/* check for the eDP connector */
> +		if(output->config.connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		{
> +			igt_skip("No eDP output found, skipping the test.\n");

Why skip here? What if next one could work?

Regards,
Kamil

> +		}
>  		/* Check VRR capabilities before setting up */
>  		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
>  		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
> @@ -618,28 +624,46 @@ static void test_deep_pkgc_state(data_t *data)
>  
>  	igt_output_set_pipe(output, pipe);
>  
> +	for_each_connector_mode(output)
> +	{
> +		data->mode = &output->config.connector->modes[j__];
> +		vb_delay = 1 * (MSECS / (data->mode->vrefresh));
> +		/* At least 5ms vblank time required to program higher
> +		 * watermark level
> +		*/
> +		if(vb_delay >  5000)
> +		{
> +			break;
> +		}
> +	}
> +
> +
>  	data->output = output;
>  	data->mode = igt_output_get_mode(output);
>  	setup_videoplayback(data);
>  
>  	primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
> -	pre_val = read_pkgc_counter(data->debugfs_root_fd);
> -	delay = 1 * (MSECS / (data->mode->vrefresh - 10));
>  
>  	igt_plane_set_fb(primary, &data->fb_rgb);
>  	igt_display_commit(&data->display);
> +	/* Waiting for the vblank to sync the frame time */
> +	igt_wait_for_vblank_count(data->drm_fd,data->display.pipes[pipe].crtc_offset,1);
> +
> +	usleep(5000);
> +	pre_val = read_pkgc_counter(data->debugfs_root_fd);
>  
>  	while (time(NULL) - start < duration) {
>  		flip = !flip;
>  		igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
>  		igt_display_commit(&data->display);
> -
> +		usleep(vb_delay);
>  		cur_val = read_pkgc_counter(data->debugfs_root_fd);
>  		if (cur_val > pre_val) {
>  			pkgc_flag = true;
>  			break;
>  		}
> -		usleep(delay);
> +
> +		usleep(5000);
>  	}
>  
>  	cleanup_dc3co_fbs(data);
> -- 
> 2.43.0
>