Re: [PATCH i-g-t] lib/kms: convert output->pending_pipe to output->pending_crtc

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 05 Mar 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 05, 2026 at 04:08:51PM +0200, Jani Nikula wrote:
>> We're finally ready to convert enum pipe pending_pipe to igt_crtc_t
>> *pending_crtc.
>> 
>> igt_output_refresh() gets changed to using crtc_index instead of the
>> pipe for crtc index mask.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  lib/igt_kms.c | 93 ++++++++++++++++++++++++++-------------------------
>>  lib/igt_kms.h |  2 +-
>>  2 files changed, 48 insertions(+), 47 deletions(-)
>> 
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 9ebf77b74d04..32f959b2bead 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -2600,8 +2600,8 @@ void igt_output_refresh(igt_output_t *output)
>>  	igt_display_t *display = output->display;
>>  	unsigned long crtc_idx_mask = 0;
>>  
>> -	if (output->pending_pipe != PIPE_NONE)
>> -		crtc_idx_mask = 1 << output->pending_pipe;
>> +	if (output->pending_crtc)
>> +		crtc_idx_mask = 1 << output->pending_crtc->crtc_index;
>>  
>>  	kmstest_free_connector_config(&output->config);
>>  
>> @@ -2620,8 +2620,8 @@ void igt_output_refresh(igt_output_t *output)
>>  		igt_atomic_fill_connector_props(display, output,
>>  			IGT_NUM_CONNECTOR_PROPS, igt_connector_prop_names);
>>  
>> -	LOG(display, "%s: Selecting pipe %s\n", output->name,
>> -	    kmstest_pipe_name(output->pending_pipe));
>> +	LOG(display, "%s: Selecting CRTC %s\n", output->name,
>> +	    igt_crtc_name(output->pending_crtc));
>>  }
>>  
>>  static int
>> @@ -2741,7 +2741,7 @@ static void igt_crtc_reset(igt_crtc_t *crtc)
>>  
>>  static void igt_output_reset(igt_output_t *output)
>>  {
>> -	output->pending_pipe = PIPE_NONE;
>> +	output->pending_crtc = NULL;
>>  	output->use_override_mode = false;
>>  	memset(&output->override_mode, 0, sizeof(output->override_mode));
>>  
>> @@ -2932,7 +2932,7 @@ void igt_display_reset_outputs(igt_display_t *display)
>>  		 * We don't assign each output a pipe unless
>>  		 * a CRTC is set with igt_output_set_crtc().
>>  		 */
>> -		output->pending_pipe = PIPE_NONE;
>> +		output->pending_crtc = NULL;
>>  		output->id = resources->connectors[i];
>>  		output->display = display;
>>  
>> @@ -3438,19 +3438,20 @@ void igt_display_fini(igt_display_t *display)
>>  static void igt_display_refresh(igt_display_t *display)
>>  {
>>  	igt_output_t *output;
>> +	unsigned int crtc_index_in_use_mask = 0;
>>  	int i;
>>  
>> -	unsigned long pipes_in_use = 0;
>> -
>>         /* Check that two outputs aren't trying to use the same pipe */
>>  	for (i = 0; i < display->n_outputs; i++) {
>>  		output = &display->outputs[i];
>>  
>> -		if (output->pending_pipe != PIPE_NONE) {
>> -			if (pipes_in_use & (1 << output->pending_pipe))
>> +		if (output->pending_crtc) {
>> +			unsigned int crtc_index_mask = 1 << output->pending_crtc->crtc_index;
>> +
>> +			if (crtc_index_in_use_mask & crtc_index_mask)
>>  				goto report_dup;
>>  
>> -			pipes_in_use |= 1 << output->pending_pipe;
>> +			crtc_index_in_use_mask |= crtc_index_mask;
>>  		}
>>  
>>  		if (output->force_reprobe)
>> @@ -3463,35 +3464,23 @@ report_dup:
>>  	for (; i > 0; i--) {
>>  		igt_output_t *b = &display->outputs[i - 1];
>>  
>> -		igt_assert_f(output->pending_pipe !=
>> -			     b->pending_pipe,
>> -			     "%s and %s are both trying to use pipe %s\n",
>> +		if (!b->pending_crtc)
>> +			continue;
>> +
>> +		igt_assert_f(output->pending_crtc != b->pending_crtc,
>> +			     "%s and %s are both trying to use CRTC %s\n",
>>  			     igt_output_name(output), igt_output_name(b),
>> -			     kmstest_pipe_name(output->pending_pipe));
>> +			     igt_crtc_name(output->pending_crtc));
>>  	}
>>  }
>>  
>> +/*
>> + * Return the pending CRTC (i.e. the CRTC that should drive this output after
>> + * the commit(), or NULL if the user hasn't specified a CRTC to use.
>> + */
>>  igt_crtc_t *igt_output_get_driving_crtc(igt_output_t *output)
>>  {
>> -	igt_display_t *display = output->display;
>> -	enum pipe pipe;
>> -
>> -	if (output->pending_pipe == PIPE_NONE) {
>> -		/*
>> -		 * The user hasn't specified a pipe to use, return none.
>> -		 */
>> -		return NULL;
>> -	} else {
>> -		/*
>> -		 * Otherwise, return the pending pipe (ie the pipe that should
>> -		 * drive this output after the commit()
>> -		 */
>> -		pipe = output->pending_pipe;
>> -	}
>> -
>> -	igt_assert(pipe >= 0 && pipe < igt_display_n_crtcs(display));
>> -
>> -	return igt_crtc_for_pipe(display, pipe);
>> +	return output->pending_crtc;
>>  }
>>  
>>  static igt_plane_t *igt_crtc_get_plane(igt_crtc_t *crtc, int plane_idx)
>> @@ -3685,10 +3674,13 @@ static igt_output_t *igt_crtc_get_output(igt_crtc_t *crtc)
>>  	igt_display_t *display = crtc->display;
>>  	int i;
>>  
>> +	if (!crtc)
>
> I wasn't expecting NULL here, and we dereference crtc->display
> above already. Is there some place we might pass NULL?

Right, I just wanted to avoid the below loop matching two NULL crtcs,
but that obviously can't happen due to the crtc->display dereference.

I'll remove the check.


>
>> +		return NULL;
>> +
>>  	for (i = 0; i < display->n_outputs; i++) {
>>  		igt_output_t *output = &display->outputs[i];
>>  
>> -		if (output->pending_pipe == crtc->pipe)
>> +		if (output->pending_crtc == crtc)
>>  			return output;
>>  	}
>>  
>> @@ -5285,12 +5277,21 @@ void igt_output_set_crtc(igt_output_t *output, igt_crtc_t *crtc)
>>  
>>  	igt_assert(output->name);
>>  
>> -	if (output->pending_pipe != PIPE_NONE)
>> +	if (output->pending_crtc)
>>  		old_crtc = igt_output_get_driving_crtc(output);
>>  
>> +	/*
>> +	 * Ensure pending_crtc is always valid.
>> +	 *
>> +	 * FIXME: Ensure we only have valid crtc objects around in general, so
>> +	 * we can remove this check.
>> +	 */
>> +	if (!crtc->valid)
>> +		crtc = NULL;

OTOH, this one does need to check for crtc != NULL before dereferencing
because NULL crtc is explicitly passed to igt_output_set_crtc().

BR,
Jani.


>> +
>>  	LOG(display, "%s: set_crtc(%s)\n", igt_output_name(output),
>>  	    igt_crtc_name(crtc));
>> -	output->pending_pipe = crtc ? crtc->pipe : PIPE_NONE;
>> +	output->pending_crtc = crtc;
>>  
>>  	if (old_crtc) {
>>  		igt_output_t *old_output;

-- 
Jani Nikula, Intel