| Message ID | 20240315170959.165505-3-sunpeng.li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu/display: Make multi-plane configurations more flexible | expand |
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); > }
On Fri, 15 Mar 2024 13:09:58 -0400 <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. This sounds good to me too. I suppose that means Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> for both patches. Or at least for their intentions. :-) Thanks, pq > Signed-off-by: Leo Li <sunpeng.li@amd.com> > --- > .../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) \ + 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); }