[PATCH 7/7] drm/amd/display: Replace DRM private objects with subclassed DRM atomic state
Kazlauskas, Nicholas
nicholas.kazlauskas at amd.com
Thu Aug 6 14:25:10 UTC 2020
On 2020-08-05 4:37 p.m., Rodrigo Siqueira wrote:
> 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.
Thanks for the reviews!
I can clean up the nitpicks for this patch and make a v2.
Regards,
Nicholas Kazlauskas
>
> 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
>
More information about the dri-devel
mailing list