Re: [PATCH 2/2] drm/amd/display: Move PRIMARY plane zpos higher

[Harry Wentland <harry.wentland@amd.com>]

On 2024-03-15 13:09, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> Compositors have different ways of assigning surfaces to DRM planes for
> render offloading. It may decide between various strategies: overlay,
> underlay, or a mix of both
> 
> One way for compositors to implement the underlay strategy is to assign
> a higher zpos to the DRM_PRIMARY plane than the DRM_OVERLAY planes,
> effectively turning the DRM_OVERLAY plane into an underlay plane.
> 
> Today, amdgpu attaches an immutable zpos of 0 to the DRM_PRIMARY plane.
> This however, is an arbitrary restriction. DCN pipes are general
> purpose, and can be arranged in any z-order. To support compositors
> using this allocation scheme, we can set a non-zero immutable zpos for
> the PRIMARY, allowing the placement of OVERLAYS (mutable zpos range
> 0-254) beneath the PRIMARY.
> 
> [How]
> 
> Assign a zpos = #no of OVERLAY planes to the PRIMARY plane. Then, clean
> up any assumptions in the driver of PRIMARY plane having the lowest
> zpos.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

With the typo mentioned below fixes this is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Before merging we should run a full promotion test (especially the IGT
tests) on it as this could break things in subtle ways.

Harry

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 96 ++++++++++++++++++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 17 +++-
>  2 files changed, 104 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 09ab330aed17..01b00f587701 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -80,6 +80,7 @@
>  #include <linux/firmware.h>
>  #include <linux/component.h>
>  #include <linux/dmi.h>
> +#include <linux/sort.h>
>  
>  #include <drm/display/drm_dp_mst_helper.h>
>  #include <drm/display/drm_hdmi_helper.h>
> @@ -369,6 +370,20 @@ static inline void reverse_planes_order(struct dc_surface_update *array_of_surfa
>  		swap(array_of_surface_update[i], array_of_surface_update[j]);
>  }
>  
> +/*
> + * DC will program planes with their z-order determined by their ordering
> + * in the dc_surface_updates array. This comparator is used to sort them
> + * by descending zpos.
> + */
> +static int dm_plane_layer_index_cmp(const void *a, const void *b)
> +{
> +	const struct dc_surface_update *sa = (struct dc_surface_update *)a;
> +	const struct dc_surface_update *sb = (struct dc_surface_update *)b;
> +
> +	/* Sort by descending dc_plane layer_index (i.e. normalized_zpos) */
> +	return sb->surface->layer_index - sa->surface->layer_index;
> +}
> +
>  /**
>   * update_planes_and_stream_adapter() - Send planes to be updated in DC
>   *
> @@ -393,7 +408,8 @@ static inline bool update_planes_and_stream_adapter(struct dc *dc,
>  						    struct dc_stream_update *stream_update,
>  						    struct dc_surface_update *array_of_surface_update)
>  {
> -	reverse_planes_order(array_of_surface_update, planes_count);
> +	sort(array_of_surface_update, planes_count,
> +	     sizeof(*array_of_surface_update), dm_plane_layer_index_cmp, NULL);
>  
>  	/*
>  	 * Previous frame finished and HW is ready for optimization.
> @@ -9363,6 +9379,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  		for (j = 0; j < status->plane_count; j++)
>  			dummy_updates[j].surface = status->plane_states[0];
>  
> +		sort(dummy_updates, status->plane_count,
> +		     sizeof(*dummy_updates), dm_plane_layer_index_cmp, NULL);
>  
>  		mutex_lock(&dm->dc_lock);
>  		dc_update_planes_and_stream(dm->dc,
> @@ -10097,6 +10115,17 @@ static bool should_reset_plane(struct drm_atomic_state *state,
>  	if (new_crtc_state->color_mgmt_changed)
>  		return true;
>  
> +	/*
> +	 * On zpos change, planes need to be reordered by removing and re-adding
> +	 * them one by one to the dc state, in order of descending zpos.
> +	 *
> +	 * TODO: We can likely skip bandwidth validation if the only thing that
> +	 * changed about the plane was it'z z-ordering.
> +	 */
> +	if (new_crtc_state->zpos_changed) {
> +		return true;
> +	}
> +
>  	if (drm_atomic_crtc_needs_modeset(new_crtc_state))
>  		return true;
>  
> @@ -10509,6 +10538,65 @@ dm_get_plane_scale(struct drm_plane_state *plane_state,
>  	*out_plane_scale_h = plane_state->crtc_h * 1000 / plane_src_h;
>  }
>  
> +/*
> + * The normalized_zpos value cannot be used by this iterator directly. It's only
> + * calculated for enabled planes, potentially causing normalized_zpos collisions
> + * between enabled/disabled planes in the atomic state. We need a unique value
> + * so that the iterator will not generate the same object twice, or loop
> + * indefinitely.
> + */
> +static inline struct __drm_planes_state *__get_next_zpos(
> +	struct drm_atomic_state *state,
> +	struct __drm_planes_state *prev)
> +{
> +	unsigned int highest_zpos = 0, prev_zpos = 256;
> +	uint32_t highest_id = 0, prev_id = UINT_MAX;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_plane *plane;
> +	int i, highest_i = -1;
> +
> +	if (prev != NULL) {
> +		prev_zpos = prev->new_state->zpos;
> +		prev_id = prev->ptr->base.id;
> +	}
> +
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		/* Skip planes with higher zpos than the previously returned */
> +		if (new_plane_state->zpos > prev_zpos ||
> +		    (new_plane_state->zpos == prev_zpos &&
> +		     plane->base.id >= prev_id))
> +			continue;
> +
> +		/* Save the index of the plane with highest zpos */
> +		if (new_plane_state->zpos > highest_zpos ||
> +		    (new_plane_state->zpos == highest_zpos &&
> +		     plane->base.id > highest_id)) {
> +			highest_zpos = new_plane_state->zpos;
> +			highest_id = plane->base.id;
> +			highest_i = i;
> +		}
> +	}
> +
> +	if (highest_i < 0)
> +		return NULL;
> +
> +	return &state->planes[highest_i];
> +}
> +
> +/*
> + * Use the uniqueness of the plane's (zpos, drm obj ID) combination to iterate
> + * by descending zpos, as read from the new plane state. This is the same
> + * ordering as defined by drm_atomic_normalize_zpos().
> + */
> +#define for_each_oldnew_plane_in_decending_zpos(__state, plane, old_plane_state, new_plane_state) \

"decending" > "descending"

Harry

> +	for (struct __drm_planes_state *__i = __get_next_zpos((__state), NULL); \
> +	     __i != NULL; __i = __get_next_zpos((__state), __i))		\
> +		for_each_if (((plane) = __i->ptr,			\
> +			      (void)(plane) /* Only to avoid unused-but-set-variable warning */, \
> +			      (old_plane_state) = __i->old_state,	\
> +			      (new_plane_state) = __i->new_state, 1))
> +
> +
>  static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_crtc_state *new_crtc_state)
> @@ -10571,7 +10659,7 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>  	if (i)
>  		return i;
>  
> -	for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
> +	for_each_oldnew_plane_in_decending_zpos(state, underlying, old_plane_state, new_underlying_state) {
>  		/* Narrow down to non-cursor planes on the same CRTC as the cursor */
>  		if (new_underlying_state->crtc != crtc || underlying == crtc->cursor)
>  			continue;
> @@ -10936,7 +11024,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  	}
>  
>  	/* Remove exiting planes if they are modified */
> -	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> +	for_each_oldnew_plane_in_decending_zpos(state, plane, old_plane_state, new_plane_state) {
>  		if (old_plane_state->fb && new_plane_state->fb &&
>  		    get_mem_type(old_plane_state->fb) !=
>  		    get_mem_type(new_plane_state->fb))
> @@ -10981,7 +11069,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  	}
>  
>  	/* Add new/modified planes */
> -	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> +	for_each_oldnew_plane_in_decending_zpos(state, plane, old_plane_state, new_plane_state) {
>  		ret = dm_update_plane_state(dc, state, plane,
>  					    old_plane_state,
>  					    new_plane_state,
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index ed1fc01f1649..787c0dcdd1ea 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -104,8 +104,6 @@ void amdgpu_dm_plane_fill_blending_from_plane_state(const struct drm_plane_state
>  	*global_alpha = false;
>  	*global_alpha_value = 0xff;
>  
> -	if (plane_state->plane->type == DRM_PLANE_TYPE_PRIMARY)
> -		return;
>  
>  	if (plane_state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI ||
>  		plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE) {
> @@ -1686,6 +1684,7 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  	int res = -EPERM;
>  	unsigned int supported_rotations;
>  	uint64_t *modifiers = NULL;
> +	unsigned int primary_zpos = dm->dc->caps.max_slave_planes;
>  
>  	num_formats = amdgpu_dm_plane_get_plane_formats(plane, plane_cap, formats,
>  							ARRAY_SIZE(formats));
> @@ -1715,10 +1714,18 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  	}
>  
>  	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> -		drm_plane_create_zpos_immutable_property(plane, 0);
> +		/*
> +		 * Allow OVERLAY planes to be used as underlays by assigning an
> +		 * immutable zpos = # of OVERLAY planes to the PRIMARY plane.
> +		 */
> +		drm_plane_create_zpos_immutable_property(plane, primary_zpos);
>  	} else if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> -		unsigned int zpos = 1 + drm_plane_index(plane);
> -		drm_plane_create_zpos_property(plane, zpos, 1, 254);
> +		/*
> +		 * OVERLAY planes can be below or above the PRIMARY, but cannot
> +		 * be above the CURSOR plane.
> +		 */
> +		unsigned int zpos = primary_zpos + 1 + drm_plane_index(plane);
> +		drm_plane_create_zpos_property(plane, zpos, 0, 254);
>  	} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>  		drm_plane_create_zpos_immutable_property(plane, 255);
>  	}