[PATCH 5/5] drm/amd/display: Move the dm update dance to crtc->atomic_check
Grodzovsky, Andrey
Andrey.Grodzovsky at amd.com
Tue Dec 18 20:33:37 UTC 2018
On 12/18/2018 12:09 PM, Kazlauskas, Nicholas wrote:
> On 12/18/18 10:26 AM, sunpeng.li at amd.com wrote:
>> From: Leo Li <sunpeng.li at amd.com>
>>
>> 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 at amd.com>
>> ---
>> 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.
I believe (but not 100%) that it should be OK since this is actually
more aligned with how DC works in a sense that planes always belong to
some stream (stream_status) so for each stream you will be handling all
it's planes at once.
Of course it would be much better if there was no such constraint in DC
and we could create/modify/destroy planes as independent from CRTC
entities - then we could simply put the plane update logic in
dm_plane_atomic_check and CRTC update logic dm_crtc_helper_atomic_check
which then would be truly aligned with DRM - but this I think is tricky
from DC design perspective.
Andrey
>
>
>> +
>> + 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list