Re: [igt-dev] [PATCH i-g-t] lib/igt_kms: Don't reset VRR_ENABLED on every commit

From: Petri Latvala <petri.latvala@intel.com>
To: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t] lib/igt_kms: Don't reset VRR_ENABLED on every commit
Date: Thu, 31 Jan 2019 12:33:04 +0200	[thread overview]
Message-ID: <20190131103304.GK4038@platvala-desk.ger.corp.intel.com> (raw)
In-Reply-To: <20190130151408.4571-1-nicholas.kazlauskas@amd.com>

On Wed, Jan 30, 2019 at 10:14:08AM -0500, Nicholas Kazlauskas wrote:
> If the VRR tests failed then there was a chance that VRR could be
> left enabled when exiting the test so the VRR_ENABLED property was
> reset to 0 whenever the pipe was reset.
> 
> However, in doing so the pipe's state was considered changed even
> if VRR_ENABLED was already 0. This causes the pipe to be added to
> commits where it previously wasn't.
> 
> Other tests with properties that can persist for failures (like color
> management) don't bother resetting the properties - so this patch
> changes VRR_ENABLED to work the same.
> 
> The VRR_ENABLED property was also never cleared after being set and
> commit - leading to similar issues as the one described above. This
> patch now clears it after being commit.
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---

Thanks for taking a look. This change with VRR_ENABLED uncovered a
rabbit hole that now looks sufficiently taken care of. Maarten's more
aggressive change to display_commit_changed makes this one require a
(trivial) rebase.

>  lib/igt_kms.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 71df98d2..1c6aa754 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1791,9 +1791,6 @@ static void igt_pipe_reset(igt_pipe_t *pipe)
>  	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_ACTIVE, 0);
>  	igt_pipe_obj_clear_prop_changed(pipe, IGT_CRTC_OUT_FENCE_PTR);
>  
> -	if (igt_pipe_obj_has_prop(pipe, IGT_CRTC_VRR_ENABLED))
> -		igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_VRR_ENABLED, 0);
> -
>  	pipe->out_fence_fd = -1;
>  }
>  
> @@ -3227,6 +3224,7 @@ display_commit_changed(igt_display_t *display, enum igt_commit_style s)
>  			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_CTM);
>  			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_DEGAMMA_LUT);
>  			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_GAMMA_LUT);
> +			igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_VRR_ENABLED);
>  
>  			if (s != COMMIT_UNIVERSAL) {
>  				igt_pipe_obj_clear_prop_changed(pipe_obj, IGT_CRTC_MODE_ID);

With Maarten's patch, this last chunk needs to be dropped. With that,

Reviewed-by: Petri Latvala <petri.latvala@intel.com>

I'll merge when Maarten's patch lands, which is just waiting for CI to
churn through it.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev