Re: [PATCH i-g-t 11/11] lib/kms: Streamline plane index handling further

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

On Wed, 17 Dec 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Eliminate some redundant plane type checks and just rely
> on plane_type_index() knowing what it's doing.
>
> We do need to keep one plane type check to determing
> what to do with pipe->num_primary_plane and
> pipe->has_cursor_plane. But it's a bit more obvious
> what's happening there now that index stuff isn't mixed in.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  lib/igt_kms.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 4f30732bf1bb..04a86f235471 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3075,24 +3075,26 @@ static void igt_crtc_plane_init(igt_display_t *display,
>  	drmModePlane *drm_plane = global_plane->drm_plane;
>  	int type = global_plane->type;
>  	igt_plane_t *plane;
> -	int index = plane_type_index(pipe, type);
> +	int index;
>  
> -	if (type == DRM_PLANE_TYPE_PRIMARY && pipe->planes[index].index < 0) {
> +	switch (type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
>  		pipe->num_primary_planes++;
> -	} else if (type == DRM_PLANE_TYPE_CURSOR && pipe->planes[index].index < 0) {
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
>  		display->has_cursor_plane = true;
> -	} else {
> +		break;
> +	default:
> +		break;
> +	}

Plain old if would be much shorter...

	if (type == DRM_PLANE_TYPE_PRIMARY)
		pipe->num_primary_planes++;
	else if (type == DRM_PLANE_TYPE_CURSOR)
		display->has_cursor_plane = true;

But *shrug*.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
> +	index = plane_type_index(pipe, type);
> +
> +	if (index < 0 || pipe->planes[index].index >= 0) {
>  		for (index = 1; index < pipe->n_planes; index++) {
>  			if (pipe->planes[index].index < 0)
>  				break;
>  		}
> -
> -		/*
> -		 * Increment num_primary_planes for any extra
> -		 * primary plane found.
> -		 */
> -		if (type == DRM_PLANE_TYPE_PRIMARY)
> -			pipe->num_primary_planes++;
>  	}
>  
>  	igt_assert_lt(index, pipe->n_planes);
> @@ -3576,20 +3578,13 @@ static igt_plane_t *igt_pipe_get_plane(igt_crtc_t *pipe, int plane_idx)
>   */
>  igt_plane_t *igt_pipe_get_plane_type(igt_crtc_t *pipe, int plane_type)
>  {
> -	int i, plane_idx = -1;
> +	int plane_idx = plane_type_index(pipe, plane_type);
>  
> -	switch(plane_type) {
> -	case DRM_PLANE_TYPE_CURSOR:
> -	case DRM_PLANE_TYPE_PRIMARY:
> -		plane_idx = plane_type_index(pipe, plane_type);
> -		break;
> -	case DRM_PLANE_TYPE_OVERLAY:
> -		for(i = 0; i < pipe->n_planes; i++)
> -			if (pipe->planes[i].type == DRM_PLANE_TYPE_OVERLAY)
> -			    plane_idx = i;
> -		break;
> -	default:
> -		break;
> +	if (plane_idx < 0) {
> +		for (plane_idx = 0; plane_idx < pipe->n_planes; plane_idx++) {
> +			if (pipe->planes[plane_idx].type == plane_type)
> +				break;
> +		}
>  	}
>  
>  	igt_require_f(plane_idx >= 0 && plane_idx < pipe->n_planes,

-- 
Jani Nikula, Intel