[PATCH 7/7] drm/amd/display: Replace DRM private objects with subclassed DRM atomic state
Rodrigo Siqueira
Rodrigo.Siqueira at amd.com
Wed Aug 5 20:37:33 UTC 2020
Hi,
I have some minor inline comments, but everything looks fine when I
tested it on Raven; feel free to add
Tested-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
in the whole series.
On 07/30, Nicholas Kazlauskas wrote:
> [Why]
> DM atomic check was structured in a way that we required old DC state
> in order to dynamically add and remove planes and streams from the
> context to build the DC state context for validation.
>
> DRM private objects were used to carry over the last DC state and
> were added to the context on nearly every commit - regardless of fast
> or full so we could check whether or not the new state could affect
> bandwidth.
>
> The problem with this model is that DRM private objects do not
> implicitly stall out other commits.
>
> So if you have two commits touching separate DRM objects they could
> run concurrently and potentially execute out of order - leading to a
> use-after-free.
>
> If we want this to be safe we have two options:
> 1. Stall out concurrent commits since they touch the same private object
> 2. Refactor DM to not require old DC state and drop private object usage
>
> [How]
> This implements approach #2 since it still allows for judder free
> updates in multi-display scenarios.
>
> I'll list the big changes in order at a high level:
>
> 1. Subclass DRM atomic state instead of using DRM private objects.
>
> DC relied on the old state to determine which changes cause bandwidth
> updates but now we have DM perform similar checks based on DRM state
> instead - dropping the requirement for old state to exist at all.
>
> This means that we can now build a new DC context from scratch whenever
> we have something that DM thinks could affect bandwidth.
>
> Whenever we need to rebuild bandwidth we now add all CRTCs and planes
> to the DRM state in order to get the absolute set of DC streams and
> DC planes.
>
> This introduces a stall on other commits, but this stall already
> exists because of the lock_and_validation logic and it's necessary
> since updates may move around pipes and require full reprogramming.
>
> 2. Drop workarounds to add planes to maintain z-order early in atomic
> check. This is no longer needed because of the changes for (1).
>
> This also involves fixing up should_plane_reset checks since we can just
> avoid resetting streams and planes when they haven't actually changed.
>
> 3. Rework dm_update_crtc_state and dm_update_plane_state to be single
> pass instead of two pass.
>
> This is necessary since we no longer have the dc_state to add and
> remove planes to the context in and we want to defer creation to the
> end of commit_check.
>
> It also makes the logic a lot simpler to follow since as an added bonus.
>
> Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 720 +++++++-----------
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 11 +-
> 2 files changed, 280 insertions(+), 451 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 59829ec81694..97a7dfc620e8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1839,7 +1839,6 @@ static int dm_resume(void *handle)
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> struct dm_plane_state *dm_new_plane_state;
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(dm->atomic_obj.state);
> enum dc_connection_type new_connection_type = dc_connection_none;
> struct dc_state *dc_state;
> int i, r, j;
> @@ -1879,11 +1878,6 @@ static int dm_resume(void *handle)
>
> return 0;
> }
> - /* Recreate dc_state - DC invalidates it when setting power state to S3. */
> - dc_release_state(dm_state->context);
> - dm_state->context = dc_create_state(dm->dc);
> - /* TODO: Remove dc_state->dccg, use dc->dccg directly. */
> - dc_resource_state_construct(dm->dc, dm_state->context);
>
> /* Before powering on DC we need to re-initialize DMUB. */
> r = dm_dmub_hw_init(adev);
> @@ -2019,11 +2013,51 @@ const struct amdgpu_ip_block_version dm_ip_block =
> * *WIP*
> */
>
> +struct drm_atomic_state *dm_atomic_state_alloc(struct drm_device *dev)
> +{
> + struct dm_atomic_state *dm_state;
> +
> + dm_state = kzalloc(sizeof(*dm_state), GFP_KERNEL);
How about use GFP_ATOMIC here?
> +
> + if (!dm_state)
> + return NULL;
> +
> + if (drm_atomic_state_init(dev, &dm_state->base) < 0) {
> + kfree(dm_state);
> + return NULL;
> + }
> +
> + return &dm_state->base;
> +}
> +
> +void dm_atomic_state_free(struct drm_atomic_state *state)
> +{
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> +
> + if (dm_state->context) {
> + dc_release_state(dm_state->context);
> + dm_state->context = NULL;
> + }
> +
> + drm_atomic_state_default_release(state);
> + kfree(state);
> +}
> +
> +void dm_atomic_state_clear(struct drm_atomic_state *state)
> +{
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> +
> + drm_atomic_state_default_clear(&dm_state->base);
> +}
> +
> static const struct drm_mode_config_funcs amdgpu_dm_mode_funcs = {
> .fb_create = amdgpu_display_user_framebuffer_create,
> .output_poll_changed = drm_fb_helper_output_poll_changed,
> .atomic_check = amdgpu_dm_atomic_check,
> .atomic_commit = amdgpu_dm_atomic_commit,
> + .atomic_state_alloc = dm_atomic_state_alloc,
Nit: the above hooks use the prefix amdgpu_dm, maybe it is a good idea
to keep this pattern for the new functions.
> + .atomic_state_free = dm_atomic_state_free,
> + .atomic_state_clear = dm_atomic_state_clear,
Looks like that the above function scope is restricted to this file and
you only use it in the above data struct. How about making all of the
new functions static?
> };
>
> static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
> @@ -2782,108 +2816,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> }
> #endif
>
> -/*
> - * Acquires the lock for the atomic state object and returns
> - * the new atomic state.
> - *
> - * This should only be called during atomic check.
> - */
> -static int dm_atomic_get_state(struct drm_atomic_state *state,
> - struct dm_atomic_state **dm_state)
> -{
> - struct drm_device *dev = state->dev;
> - struct amdgpu_device *adev = dev->dev_private;
> - struct amdgpu_display_manager *dm = &adev->dm;
> - struct drm_private_state *priv_state;
> -
> - if (*dm_state)
> - return 0;
> -
> - priv_state = drm_atomic_get_private_obj_state(state, &dm->atomic_obj);
> - if (IS_ERR(priv_state))
> - return PTR_ERR(priv_state);
> -
> - *dm_state = to_dm_atomic_state(priv_state);
> -
> - return 0;
> -}
> -
> -static struct dm_atomic_state *
> -dm_atomic_get_new_state(struct drm_atomic_state *state)
> -{
> - struct drm_device *dev = state->dev;
> - struct amdgpu_device *adev = dev->dev_private;
> - struct amdgpu_display_manager *dm = &adev->dm;
> - struct drm_private_obj *obj;
> - struct drm_private_state *new_obj_state;
> - int i;
> -
> - for_each_new_private_obj_in_state(state, obj, new_obj_state, i) {
> - if (obj->funcs == dm->atomic_obj.funcs)
> - return to_dm_atomic_state(new_obj_state);
> - }
> -
> - return NULL;
> -}
> -
> -static struct dm_atomic_state *
> -dm_atomic_get_old_state(struct drm_atomic_state *state)
> -{
> - struct drm_device *dev = state->dev;
> - struct amdgpu_device *adev = dev->dev_private;
> - struct amdgpu_display_manager *dm = &adev->dm;
> - struct drm_private_obj *obj;
> - struct drm_private_state *old_obj_state;
> - int i;
> -
> - for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
> - if (obj->funcs == dm->atomic_obj.funcs)
> - return to_dm_atomic_state(old_obj_state);
> - }
> -
> - return NULL;
> -}
> -
> -static struct drm_private_state *
> -dm_atomic_duplicate_state(struct drm_private_obj *obj)
> -{
> - struct dm_atomic_state *old_state, *new_state;
> -
> - new_state = kzalloc(sizeof(*new_state), GFP_KERNEL);
> - if (!new_state)
> - return NULL;
> -
> - __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base);
> -
> - old_state = to_dm_atomic_state(obj->state);
> -
> - if (old_state && old_state->context)
> - new_state->context = dc_copy_state(old_state->context);
> -
> - if (!new_state->context) {
> - kfree(new_state);
> - return NULL;
> - }
> -
> - return &new_state->base;
> -}
> -
> -static void dm_atomic_destroy_state(struct drm_private_obj *obj,
> - struct drm_private_state *state)
> -{
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> -
> - if (dm_state && dm_state->context)
> - dc_release_state(dm_state->context);
> -
> - kfree(dm_state);
> -}
> -
> -static struct drm_private_state_funcs dm_atomic_state_funcs = {
> - .atomic_duplicate_state = dm_atomic_duplicate_state,
> - .atomic_destroy_state = dm_atomic_destroy_state,
> -};
> -
> static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> {
> struct dm_atomic_state *state;
> @@ -2916,11 +2848,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>
> dc_resource_state_copy_construct_current(adev->dm.dc, state->context);
>
> - drm_atomic_private_obj_init(adev->ddev,
> - &adev->dm.atomic_obj,
> - &state->base,
> - &dm_atomic_state_funcs);
> -
> r = amdgpu_display_modeset_create_props(adev);
> if (r)
> return r;
> @@ -3360,7 +3287,6 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
> static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm)
> {
> drm_mode_config_cleanup(dm->ddev);
> - drm_atomic_private_obj_fini(&dm->atomic_obj);
> return;
> }
>
> @@ -7533,7 +7459,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_display_manager *dm = &adev->dm;
> - struct dm_atomic_state *dm_state;
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> struct dc_state *dc_state = NULL, *dc_state_temp = NULL;
> uint32_t i, j;
> struct drm_crtc *crtc;
> @@ -7547,8 +7473,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>
> drm_atomic_helper_update_legacy_modeset_state(dev, state);
>
> - dm_state = dm_atomic_get_new_state(state);
> - if (dm_state && dm_state->context) {
> + if (dm_state->context) {
> dc_state = dm_state->context;
> } else {
> /* No state changes, retain current state. */
> @@ -8052,10 +7977,8 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state,
> struct drm_crtc_state *new_crtc_state,
> - bool enable,
> bool *lock_and_validation_needed)
> {
> - struct dm_atomic_state *dm_state = NULL;
> struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
> struct dc_stream_state *new_stream;
> int ret = 0;
> @@ -8077,7 +8000,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>
> /* TODO This hack should go away */
> - if (aconnector && enable) {
> + if (aconnector) {
> /* Make sure fake sink is created in plug-in scenario */
> drm_new_conn_state = drm_atomic_get_new_connector_state(state,
> &aconnector->base);
> @@ -8155,36 +8078,20 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> new_crtc_state->active_changed,
> new_crtc_state->connectors_changed);
>
> - /* Remove stream for any changed/disabled CRTC */
> - if (!enable) {
> -
> - if (!dm_old_crtc_state->stream)
> - goto skip_modeset;
> -
> - ret = dm_atomic_get_state(state, &dm_state);
> - if (ret)
> - goto fail;
> -
> - DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
> - crtc->base.id);
> + /* Remove stream for changed CRTC - can't reuse it for validation. */
> + if (dm_new_crtc_state->stream) {
> + DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n", crtc->base.id);
>
> - /* i.e. reset mode */
> - if (dc_remove_stream_from_ctx(
> - dm->dc,
> - dm_state->context,
> - dm_old_crtc_state->stream) != DC_OK) {
> - ret = -EINVAL;
> - goto fail;
> - }
> -
> - dc_stream_release(dm_old_crtc_state->stream);
> + dc_stream_release(dm_new_crtc_state->stream);
> dm_new_crtc_state->stream = NULL;
>
> reset_freesync_config_for_crtc(dm_new_crtc_state);
>
> *lock_and_validation_needed = true;
> + }
>
> - } else {/* Add stream for any updated/enabled CRTC */
> + /* Add stream for any updated/enabled CRTC - active implies enabled. */
> + if (new_crtc_state->active) {
> /*
> * Quick fix to prevent NULL pointer on new_stream when
> * added MST connectors not found in existing crtc_state in the chained mode
> @@ -8193,35 +8100,13 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> if (!aconnector || (!aconnector->dc_sink && aconnector->mst_port))
> goto skip_modeset;
>
> - if (modereset_required(new_crtc_state))
> - goto skip_modeset;
> -
> - if (modeset_required(new_crtc_state, new_stream,
> - dm_old_crtc_state->stream)) {
> -
> - WARN_ON(dm_new_crtc_state->stream);
> -
> - ret = dm_atomic_get_state(state, &dm_state);
> - if (ret)
> - goto fail;
> -
> - dm_new_crtc_state->stream = new_stream;
> + WARN_ON(dm_new_crtc_state->stream);
> + dm_new_crtc_state->stream = new_stream;
> + dc_stream_retain(new_stream);
>
> - dc_stream_retain(new_stream);
> + DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n", crtc->base.id);
>
> - DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n",
> - crtc->base.id);
> -
> - if (dc_add_stream_to_ctx(
> - dm->dc,
> - dm_state->context,
> - dm_new_crtc_state->stream) != DC_OK) {
> - ret = -EINVAL;
> - goto fail;
> - }
> -
> - *lock_and_validation_needed = true;
> - }
> + *lock_and_validation_needed = true;
> }
>
> skip_modeset:
> @@ -8233,7 +8118,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> * We want to do dc stream updates that do not require a
> * full modeset below.
> */
> - if (!(enable && aconnector && new_crtc_state->active))
> + if (!(aconnector && new_crtc_state->enable && new_crtc_state->active))
> return 0;
> /*
> * Given above conditions, the dc state cannot be NULL because:
> @@ -8281,10 +8166,12 @@ static bool should_reset_plane(struct drm_atomic_state *state,
> struct drm_plane_state *old_plane_state,
> struct drm_plane_state *new_plane_state)
> {
> - struct drm_plane *other;
> - struct drm_plane_state *old_other_state, *new_other_state;
> struct drm_crtc_state *new_crtc_state;
> - int i;
> + struct dm_plane_state *old_dm_plane_state, *new_dm_plane_state;
> +
> + /* Cursor planes don't affect bandwidth. */
> + if (plane->type == DRM_PLANE_TYPE_CURSOR)
> + return false;
>
> /*
> * TODO: Remove this hack once the checks below are sufficient
> @@ -8312,71 +8199,50 @@ static bool should_reset_plane(struct drm_atomic_state *state,
> if (new_crtc_state->color_mgmt_changed)
> return true;
>
> + /* Plane scaling can change with a modeset, so reset. */
> if (drm_atomic_crtc_needs_modeset(new_crtc_state))
> return true;
>
> - /*
> - * If there are any new primary or overlay planes being added or
> - * removed then the z-order can potentially change. To ensure
> - * correct z-order and pipe acquisition the current DC architecture
> - * requires us to remove and recreate all existing planes.
> - *
> - * TODO: Come up with a more elegant solution for this.
> - */
> - for_each_oldnew_plane_in_state(state, other, old_other_state, new_other_state, i) {
> - struct dm_plane_state *old_dm_plane_state, *new_dm_plane_state;
> -
> - if (other->type == DRM_PLANE_TYPE_CURSOR)
> - continue;
> -
> - if (old_other_state->crtc != new_plane_state->crtc &&
> - new_other_state->crtc != new_plane_state->crtc)
> - continue;
> -
> - if (old_other_state->crtc != new_other_state->crtc)
> - return true;
> -
> - /* Src/dst size and scaling updates. */
> - if (old_other_state->src_w != new_other_state->src_w ||
> - old_other_state->src_h != new_other_state->src_h ||
> - old_other_state->crtc_w != new_other_state->crtc_w ||
> - old_other_state->crtc_h != new_other_state->crtc_h)
> - return true;
> + /* Src/dst size and scaling updates. */
> + if (old_plane_state->src_w != new_plane_state->src_w ||
> + old_plane_state->src_h != new_plane_state->src_h ||
> + old_plane_state->crtc_w != new_plane_state->crtc_w ||
> + old_plane_state->crtc_h != new_plane_state->crtc_h)
> + return true;
>
> - /* Rotation / mirroring updates. */
> - if (old_other_state->rotation != new_other_state->rotation)
> - return true;
> + /* Rotation / mirroring updates. */
> + if (old_plane_state->rotation != new_plane_state->rotation)
> + return true;
>
> - /* Blending updates. */
> - if (old_other_state->pixel_blend_mode !=
> - new_other_state->pixel_blend_mode)
> - return true;
> + /* Blending updates. */
> + if (old_plane_state->pixel_blend_mode !=
> + new_plane_state->pixel_blend_mode)
> + return true;
>
> - /* Alpha updates. */
> - if (old_other_state->alpha != new_other_state->alpha)
> - return true;
> + /* Alpha updates. */
> + if (old_plane_state->alpha != new_plane_state->alpha)
> + return true;
>
> - /* Colorspace changes. */
> - if (old_other_state->color_range != new_other_state->color_range ||
> - old_other_state->color_encoding != new_other_state->color_encoding)
> - return true;
> + /* Colorspace changes. */
> + if (old_plane_state->color_range != new_plane_state->color_range ||
> + old_plane_state->color_encoding != new_plane_state->color_encoding)
> + return true;
>
> - /* Framebuffer checks fall at the end. */
> - if (!old_other_state->fb || !new_other_state->fb)
> - continue;
> + /* Framebuffer checks fall at the end. */
> + if (!old_plane_state->fb || !new_plane_state->fb)
> + return false;
>
> - /* Pixel format changes can require bandwidth updates. */
> - if (old_other_state->fb->format != new_other_state->fb->format)
> - return true;
> + /* Pixel format changes can require bandwidth updates. */
> + if (old_plane_state->fb->format != new_plane_state->fb->format)
> + return true;
>
> - old_dm_plane_state = to_dm_plane_state(old_other_state);
> - new_dm_plane_state = to_dm_plane_state(new_other_state);
> + old_dm_plane_state = to_dm_plane_state(old_plane_state);
> + new_dm_plane_state = to_dm_plane_state(new_plane_state);
>
> - /* Tiling and DCC changes also require bandwidth updates. */
> - if (old_dm_plane_state->tiling_flags !=
> - new_dm_plane_state->tiling_flags)
> - return true;
> - }
> + /* Tiling and DCC changes also require bandwidth updates. */
> + if (old_dm_plane_state->tiling_flags !=
> + new_dm_plane_state->tiling_flags)
> + return true;
>
> return false;
> }
> @@ -8386,15 +8252,12 @@ static int dm_update_plane_state(struct dc *dc,
> struct drm_plane *plane,
> struct drm_plane_state *old_plane_state,
> struct drm_plane_state *new_plane_state,
> - bool enable,
> bool *lock_and_validation_needed)
> {
> -
> - struct dm_atomic_state *dm_state = NULL;
> struct drm_crtc *new_plane_crtc, *old_plane_crtc;
> - struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> - struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
> - struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
> + struct drm_crtc_state *new_crtc_state;
> + struct dm_crtc_state *dm_new_crtc_state;
> + struct dm_plane_state *dm_new_plane_state;
> struct amdgpu_crtc *new_acrtc;
> bool needs_reset;
> int ret = 0;
> @@ -8403,12 +8266,12 @@ static int dm_update_plane_state(struct dc *dc,
> new_plane_crtc = new_plane_state->crtc;
> old_plane_crtc = old_plane_state->crtc;
> dm_new_plane_state = to_dm_plane_state(new_plane_state);
> - dm_old_plane_state = to_dm_plane_state(old_plane_state);
>
> /*TODO Implement better atomic check for cursor plane */
> if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> - if (!enable || !new_plane_crtc ||
> - drm_atomic_plane_disabling(plane->state, new_plane_state))
> + /* Cursor disabled is always OK. */
> + if (!new_plane_crtc ||
> + drm_atomic_plane_disabling(plane->state, new_plane_state))
> return 0;
>
> new_acrtc = to_amdgpu_crtc(new_plane_crtc);
> @@ -8423,123 +8286,72 @@ static int dm_update_plane_state(struct dc *dc,
> return 0;
> }
>
> + /* Check if the plane requires a reset for bandwidth validation. */
> needs_reset = should_reset_plane(state, plane, old_plane_state,
> new_plane_state);
>
> - /* Remove any changed/removed planes */
> - if (!enable) {
> - if (!needs_reset)
> - return 0;
> -
> - if (!old_plane_crtc)
> - return 0;
> -
> - old_crtc_state = drm_atomic_get_old_crtc_state(
> - state, old_plane_crtc);
> - dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> -
> - if (!dm_old_crtc_state->stream)
> - return 0;
> + /* Exit early if the plane hasn't been trivially modified. */
> + if (!needs_reset)
> + return 0;
>
> + /**
> + * Remove exisiting plane, if any - we can't reuse it for validation
Nit: exisiting -> existing
> + * because we'd be modifying the current state applied to HW.
> + */
> + if (dm_new_plane_state->dc_state) {
> DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
> - plane->base.id, old_plane_crtc->base.id);
> -
> - ret = dm_atomic_get_state(state, &dm_state);
> - if (ret)
> - return ret;
> + plane->base.id, old_plane_crtc->base.id);
>
> - if (!dc_remove_plane_from_context(
> - dc,
> - dm_old_crtc_state->stream,
> - dm_old_plane_state->dc_state,
> - dm_state->context)) {
> -
> - ret = EINVAL;
> - return ret;
> - }
> -
> -
> - dc_plane_state_release(dm_old_plane_state->dc_state);
> + dc_plane_state_release(dm_new_plane_state->dc_state);
> dm_new_plane_state->dc_state = NULL;
>
> *lock_and_validation_needed = true;
> + }
>
> - } else { /* Add new planes */
> - struct dc_plane_state *dc_new_plane_state;
> -
> - if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> - return 0;
> -
> - if (!new_plane_crtc)
> - return 0;
> -
> - new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_crtc);
> - dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -
> - if (!dm_new_crtc_state->stream)
> - return 0;
> -
> - if (!needs_reset)
> - return 0;
> -
> - ret = dm_plane_helper_check_state(new_plane_state, new_crtc_state);
> - if (ret)
> - return ret;
> -
> - WARN_ON(dm_new_plane_state->dc_state);
> -
> - dc_new_plane_state = dc_create_plane_state(dc);
> - if (!dc_new_plane_state)
> - return -ENOMEM;
> -
> - DRM_DEBUG_DRIVER("Enabling DRM plane: %d on DRM crtc %d\n",
> - plane->base.id, new_plane_crtc->base.id);
> + /**
> + * If the plane is disabling exit early. Also serves as a guarantee
> + * that we have a framebuffer below if we do have a CRTC.
> + */
> + if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> + return 0;
>
> - ret = fill_dc_plane_attributes(
> - new_plane_crtc->dev->dev_private,
> - dc_new_plane_state,
> - new_plane_state,
> - new_crtc_state);
> - if (ret) {
> - dc_plane_state_release(dc_new_plane_state);
> - return ret;
> - }
> + /* If we don't have a CRTC then the plane is disabled. */
> + if (!new_plane_crtc)
> + return 0;
>
> - ret = dm_atomic_get_state(state, &dm_state);
> - if (ret) {
> - dc_plane_state_release(dc_new_plane_state);
> - return ret;
> - }
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_crtc);
> + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>
> - /*
> - * Any atomic check errors that occur after this will
> - * not need a release. The plane state will be attached
> - * to the stream, and therefore part of the atomic
> - * state. It'll be released when the atomic state is
> - * cleaned.
> - */
> - if (!dc_add_plane_to_context(
> - dc,
> - dm_new_crtc_state->stream,
> - dc_new_plane_state,
> - dm_state->context)) {
> + /* Don't enable the plane if there's no stream for output. */
> + if (!dm_new_crtc_state->stream)
> + return 0;
>
> - dc_plane_state_release(dc_new_plane_state);
> - return -EINVAL;
> - }
> + /**
> + * Freeing the plane will take care of freeing the dc_state
> + * on failure, so we don't need to explicitly release below.
> + */
> + dm_new_plane_state->dc_state = dc_create_plane_state(dc);
> + if (!dm_new_plane_state->dc_state)
> + return -ENOMEM;
>
> - dm_new_plane_state->dc_state = dc_new_plane_state;
> + DRM_DEBUG_DRIVER("Enabling DRM plane: %d on DRM crtc %d\n",
> + plane->base.id, new_plane_crtc->base.id);
>
> - /* Tell DC to do a full surface update every time there
> - * is a plane change. Inefficient, but works for now.
> - */
> - dm_new_plane_state->dc_state->update_flags.bits.full_update = 1;
> + ret = fill_dc_plane_attributes(new_plane_crtc->dev->dev_private,
> + dm_new_plane_state->dc_state,
> + new_plane_state, new_crtc_state);
> + if (ret)
> + return ret;
>
> - *lock_and_validation_needed = true;
> - }
> + /**
> + * Tell DC to do a full surface update every time there
> + * is a plane change. Inefficient, but works for now.
> + */
> + dm_new_plane_state->dc_state->update_flags.bits.full_update = 1;
>
> + *lock_and_validation_needed = true;
>
> - return ret;
> + return 0;
> }
>
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> @@ -8567,6 +8379,113 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
> }
> #endif
>
> +static int dm_atomic_state_init_context(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> + struct amdgpu_device *adev = dev->dev_private;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> + struct dm_crtc_state *new_dm_crtc_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *new_plane_state, *old_plane_state;
> + struct dm_plane_state *new_dm_plane_state;
> + int ret, i;
> +
> + dm_state->context = dc_create_state(adev->dm.dc);
> + if (!dm_state->context)
> + return -ENOMEM;
> +
> + /**
> + * DC validation requires an absolute set of streams and planes to work
> + * with so add all planes and CRTCs to the state to make this work.
> + * Pipe allocation can change so there's no easy way to work around
> + * this constraint.
> + *
> + * Unfortunately this also means that whenever userspace requests a
> + * change that only needs to modify one CRTC's planes we still have to
> + * stall out fast updates affecting other CRTCs - introducing judder
> + * in multi-monitor scenarios.
> + *
> + * Userspace should avoid frequent updates to properties that can
> + * require bandwidth changes.
> + */
> + drm_for_each_crtc(crtc, dev) {
> + new_crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(new_crtc_state))
> + return PTR_ERR(new_crtc_state);
> +
> + /**
> + * Cursor planes may not strictly be necessary here
> + * but it's probably safer to add them.
> + */
> + ret = drm_atomic_add_affected_planes(state, crtc);
> + if (ret)
> + return ret;
> + }
> +
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> + new_dm_crtc_state = to_dm_crtc_state(new_crtc_state);
> +
> + if (!new_dm_crtc_state->stream)
> + continue;
> +
> + if (dc_add_stream_to_ctx(adev->dm.dc, dm_state->context,
> + new_dm_crtc_state->stream) != DC_OK)
> + return -EINVAL;
> + }
> +
> + /**
> + * Planes are added in reverse order to ensure correct blending order
> + * in DC - planes with higher priority go first, and the cursor and
> + * MPO planes are at the very end of the list.
> + */
> + for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> + /* Cursors aren't real hardware planes. */
> + if (plane->type == DRM_PLANE_TYPE_CURSOR)
> + continue;
> +
> + if (!new_plane_state->crtc)
> + continue;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(
> + state, new_plane_state->crtc);
> +
> + if (!new_crtc_state) {
> + DRM_WARN("No CRTC state found: plane=%d crtc=%d\n",
> + plane->base.id,
> + new_plane_state->crtc->base.id);
> + return -EINVAL;
> + }
> +
> + new_dm_crtc_state = to_dm_crtc_state(new_crtc_state);
> +
> + /* Skip planes for disabled streams. */
> + if (!new_dm_crtc_state->stream)
> + continue;
> +
> + new_dm_plane_state = to_dm_plane_state(new_plane_state);
> +
> + if (!new_dm_plane_state->dc_state) {
> + DRM_WARN("No DC state found: plane=%d crtc=%d\n",
> + plane->base.id,
> + new_plane_state->crtc->base.id);
> + return -EINVAL;
> + }
> +
> + if (!dc_add_plane_to_context(
> + adev->dm.dc, new_dm_crtc_state->stream,
> + new_dm_plane_state->dc_state, dm_state->context)) {
> + DRM_DEBUG_KMS(
> + "Couldn't add plane to context: plane=%d\n",
> + plane->base.id);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> /**
> * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
> * @dev: The DRM device
> @@ -8595,7 +8514,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> struct amdgpu_device *adev = dev->dev_private;
> - struct dm_atomic_state *dm_state = NULL;
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> struct dc *dc = adev->dm.dc;
> struct drm_connector *connector;
> struct drm_connector_state *old_con_state, *new_con_state;
> @@ -8607,6 +8526,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> int ret, i;
> bool lock_and_validation_needed = false;
>
> + /**
> + * Check for modesets on CRTCs in the new state. DRM internally checks
> + * that the mode has actually changed for us as well in here, so we can
> + * avoid modesets.
> + */
> ret = drm_atomic_helper_check_modeset(dev, state);
> if (ret)
> goto fail;
> @@ -8634,6 +8558,18 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> new_crtc_state->connectors_changed = true;
> }
>
> + /**
> + * Add all required objects for performing the commit and stalling out
> + * other commits that may be touching the same resources.
> + */
> +
Nit: Join the above and below comment in a single one, and also use '/*'
instead of '/**'.
Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
Best Regards
> + /**
> + * Pass one: Add all affected CRTCs on a MST DSC topology that has a
> + * CRTC undergoing a modeset and mark mode_changed = true for each one.
> + *
> + * Optimization: DSC can only be supported on DCN2 onwards, so we can
> + * skip on earlier ASIC.
> + */
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> if (adev->asic_type >= CHIP_NAVI10) {
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -8645,6 +8581,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> }
> }
> #endif
> +
> + /* Pass two: Add connectors and planes for CRTCs as needed. */
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
> !new_crtc_state->color_mgmt_changed &&
> @@ -8663,42 +8601,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> goto fail;
> }
>
> - /*
> - * Add all primary and overlay planes on the CRTC to the state
> - * whenever a plane is enabled to maintain correct z-ordering
> - * and to enable fast surface updates.
> - */
> - drm_for_each_crtc(crtc, dev) {
> - bool modified = false;
> -
> - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> - if (plane->type == DRM_PLANE_TYPE_CURSOR)
> - continue;
> -
> - if (new_plane_state->crtc == crtc ||
> - old_plane_state->crtc == crtc) {
> - modified = true;
> - break;
> - }
> - }
> -
> - if (!modified)
> - continue;
> -
> - drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> - if (plane->type == DRM_PLANE_TYPE_CURSOR)
> - continue;
> -
> - new_plane_state =
> - drm_atomic_get_plane_state(state, plane);
> -
> - if (IS_ERR(new_plane_state)) {
> - ret = PTR_ERR(new_plane_state);
> - goto fail;
> - }
> - }
> - }
> -
> /* Prepass for updating tiling flags on new planes. */
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> struct dm_plane_state *new_dm_plane_state = to_dm_plane_state(new_plane_state);
> @@ -8710,45 +8612,21 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> goto fail;
> }
>
> - /* Remove exiting planes if they are modified */
> - for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> - ret = dm_update_plane_state(dc, state, plane,
> - old_plane_state,
> - new_plane_state,
> - false,
> - &lock_and_validation_needed);
> - if (ret)
> - goto fail;
> - }
> -
> - /* Disable all crtcs which require disable */
> + /* Update modified CRTCs. */
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> ret = dm_update_crtc_state(&adev->dm, state, crtc,
> old_crtc_state,
> new_crtc_state,
> - false,
> &lock_and_validation_needed);
> if (ret)
> goto fail;
> }
>
> - /* Enable all crtcs which require enable */
> - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> - ret = dm_update_crtc_state(&adev->dm, state, crtc,
> - old_crtc_state,
> - new_crtc_state,
> - true,
> - &lock_and_validation_needed);
> - if (ret)
> - goto fail;
> - }
> -
> - /* Add new/modified planes */
> + /* Update modified planes. */
> for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> ret = dm_update_plane_state(dc, state, plane,
> old_plane_state,
> new_plane_state,
> - true,
> &lock_and_validation_needed);
> if (ret)
> goto fail;
> @@ -8812,10 +8690,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> * DRM state directly - we can end up disabling interrupts too early
> * if we don't.
> *
> - * TODO: Remove this stall and drop DM state private objects.
> + * TODO: Remove this stall.
> */
> if (lock_and_validation_needed) {
> - ret = dm_atomic_get_state(state, &dm_state);
> + /* Create a new DC context to validate. */
> + ret = dm_atomic_state_init_context(dev, state);
> if (ret)
> goto fail;
>
> @@ -8848,47 +8727,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> ret = -EINVAL;
> goto fail;
> }
> - } else {
> - /*
> - * The commit is a fast update. Fast updates shouldn't change
> - * the DC context, affect global validation, and can have their
> - * commit work done in parallel with other commits not touching
> - * the same resource. If we have a new DC context as part of
> - * the DM atomic state from validation we need to free it and
> - * retain the existing one instead.
> - *
> - * Furthermore, since the DM atomic state only contains the DC
> - * context and can safely be annulled, we can free the state
> - * and clear the associated private object now to free
> - * some memory and avoid a possible use-after-free later.
> - */
> -
> - for (i = 0; i < state->num_private_objs; i++) {
> - struct drm_private_obj *obj = state->private_objs[i].ptr;
> -
> - if (obj->funcs == adev->dm.atomic_obj.funcs) {
> - int j = state->num_private_objs-1;
> -
> - dm_atomic_destroy_state(obj,
> - state->private_objs[i].state);
> -
> - /* If i is not at the end of the array then the
> - * last element needs to be moved to where i was
> - * before the array can safely be truncated.
> - */
> - if (i != j)
> - state->private_objs[i] =
> - state->private_objs[j];
> -
> - state->private_objs[j].ptr = NULL;
> - state->private_objs[j].state = NULL;
> - state->private_objs[j].old_state = NULL;
> - state->private_objs[j].new_state = NULL;
> -
> - state->num_private_objs = j;
> - break;
> - }
> - }
> }
>
> /* Store the overall update type for use later in atomic check. */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index ad025f5cfd3e..1c3aa7cb83b9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -217,15 +217,6 @@ struct amdgpu_display_manager {
> struct drm_device *ddev;
> u16 display_indexes_num;
>
> - /**
> - * @atomic_obj:
> - *
> - * In combination with &dm_atomic_state it helps manage
> - * global atomic state that doesn't map cleanly into existing
> - * drm resources, like &dc_context.
> - */
> - struct drm_private_obj atomic_obj;
> -
> /**
> * @dc_lock:
> *
> @@ -440,7 +431,7 @@ struct dm_crtc_state {
> #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
>
> struct dm_atomic_state {
> - struct drm_private_state base;
> + struct drm_atomic_state base;
>
> struct dc_state *context;
> };
> --
> 2.25.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7CRodrigo.Siqueira%40amd.com%7C738ef3fdc0dc4ca497ba08d834c86d68%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317382719539545&sdata=G4xAcnpJm0BewDWqHMsRXZIv7XhnCCWu3qjF5dQLXFg%3D&reserved=0
--
Rodrigo Siqueira
https://siqueira.tech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200805/9599c216/attachment-0001.sig>
More information about the dri-devel
mailing list