On 12/18/18 10:26 AM, sunpeng.li@xxxxxxx wrote:
> From: Leo Li <sunpeng.li@xxxxxxx>
>
> drm_atomic_helper_check_planes() calls the crtc atomic check helpers. In
> an attempt to better align with the DRM framework, we can move the
> entire dm_update dance to the crtc check helper (since it essentially
> checks that we can align DC states to what DRM is requesting)
>
> Signed-off-by: Leo Li <sunpeng.li@xxxxxxx>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 145 ++++++++++++----------
> 1 file changed, 80 insertions(+), 65 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 a629544..b4dd65b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3859,15 +3859,88 @@ static bool dm_lock_and_validation_needed(struct drm_atomic_state *state)
> }
>
> static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> - struct drm_crtc_state *state)
> + struct drm_crtc_state *new_crtc_state)
> {
> struct amdgpu_device *adev = crtc->dev->dev_private;
> struct dc *dc = adev->dm.dc;
> - struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state);
> + struct drm_atomic_state *state = new_crtc_state->state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> + struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> + struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(new_crtc_state);
> int ret = -EINVAL;
> + int i;
> +
> + /*
> + * Add affected connectors and planes if a connector or plane change
> + * affects the DC stream.
> + *
> + * The additional include of affected connectors shouldn't be required
> + * outside of what is already done in drm_atomic_helper_check_modeset().
> + * We currently do this because dm_update_crtcs_state() requires the new
> + * affected connector state in order to construct a new, updated stream.
> + * See create_stream_for_sink().
> + */
> + if (new_crtc_state->enable &&
> + (new_crtc_state->color_mgmt_changed ||
> + new_crtc_state->vrr_enabled)) {
> + ret = drm_atomic_add_affected_connectors(state, crtc);
> + if (ret)
> + return ret;
> +
> + ret = drm_atomic_add_affected_planes(state, crtc);
> + if (ret)
> + return ret;
> + }
> +
> + /*
> + * Remove exiting planes for this CRTC, if they are modified or removed
> + */
> + for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> + if (!(new_crtc_state->plane_mask & drm_plane_mask(plane)) &&
> + !(old_crtc_state->plane_mask & drm_plane_mask(plane)))
> + continue;
> +
> + ret = dm_update_planes_state(dc, state, plane,
> + old_plane_state,
> + new_plane_state,
> + false);
> + if (ret)
> + return ret;
> + }
> +
> + /* Disable this crtc, if required*/
> + ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> + old_crtc_state,
> + new_crtc_state,
> + false);
> + if (ret)
> + return ret;
> +
> + /* Enable this crtc, if required*/
> + ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> + old_crtc_state,
> + new_crtc_state,
> + true);
> + if (ret)
> + return ret;
> +
> + /* Add new or add back modified planes */
> + for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> + if (!(new_crtc_state->plane_mask & drm_plane_mask(plane)) &&
> + !(old_crtc_state->plane_mask & drm_plane_mask(plane)))
> + continue;
Isn't the ordering for the planes changed with this?
Before we used to add strictly reversed, but now there's an interleaving
between the reverse planes list and the CRTC list.
I'm not sure if this is matters, but it'll be different in some cases.
> +
> + ret = dm_update_planes_state(dc, state, plane,
> + old_plane_state,
> + new_plane_state,
> + true);
> + if (ret)
> + return ret;
> + }
>
> if (unlikely(!dm_crtc_state->stream &&
> - modeset_required(state, NULL, dm_crtc_state->stream))) {
> + modeset_required(new_crtc_state, NULL, dm_crtc_state->stream))) {
> WARN_ON(1);
> return ret;
> }
> @@ -4077,6 +4150,9 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
> struct dc *dc = adev->dm.dc;
> struct dm_plane_state *dm_plane_state = to_dm_plane_state(state);
>
> + if (drm_atomic_plane_disabling(plane->state, state))
> + return 0;
> +
Doesn't this change behavior?
This was only used to skip adding planes to the context before. The rest
of plane atomic check still ran every time with the call to
drm_atomic_helper_check_planes.
If we simply return 0 here, couldn't we have invalid planes as part of
the state? They'd be disabled, but they'd still be in there.
> if (!dm_plane_state->dc_state)
> return 0;
>
> @@ -5911,10 +5987,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> struct dc *dc = adev->dm.dc;
> struct drm_connector *connector;
> struct drm_connector_state *old_con_state, *new_con_state;
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> - struct drm_plane *plane;
> - struct drm_plane_state *old_plane_state, *new_plane_state;
> enum surface_update_type update_type = UPDATE_TYPE_FAST;
> enum surface_update_type overall_update_type = UPDATE_TYPE_FAST;
>
> @@ -5926,68 +5998,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> */
> bool lock_and_validation_needed = false;
>
> +
> ret = drm_atomic_helper_check_modeset(dev, state);
> if (ret)
> goto fail;
>
> - 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 &&
> - !new_crtc_state->vrr_enabled)
> - continue;
> -
> - if (!new_crtc_state->enable)
> - continue;
> -
> - ret = drm_atomic_add_affected_connectors(state, crtc);
> - if (ret)
> - return ret;
> -
> - ret = drm_atomic_add_affected_planes(state, crtc);
> - if (ret)
> - 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_planes_state(dc, state, plane,
> - old_plane_state,
> - new_plane_state,
> - false);
> - if (ret)
> - goto fail;
> - }
> -
> - /* Disable all crtcs which require disable */
> - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> - ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> - old_crtc_state,
> - new_crtc_state,
> - false);
> - 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_crtcs_state(&adev->dm, state, crtc,
> - old_crtc_state,
> - new_crtc_state,
> - true);
> - if (ret)
> - goto fail;
> - }
> -
> - /* Add new/modified planes */
> - for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> - ret = dm_update_planes_state(dc, state, plane,
> - old_plane_state,
> - new_plane_state,
> - true);
> - if (ret)
> - goto fail;
> - }
> -
> /* Run this here since we want to validate the streams we created */
This comment could probably use updating based on your changes.
> ret = drm_atomic_helper_check_planes(dev, state);
> if (ret)
>
Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
