On 2018-12-18 3:33 p.m., Grodzovsky, Andrey wrote:
>
>
> On 12/18/2018 12:09 PM, Kazlauskas, Nicholas wrote:
>> 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.
>
> 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
I think the ordering was mainly for MPO, where DC understands the Z
order in reverse of what DRM says (not 100% certain on this either). As
Andrey says, this should be fine as we're handling all planes for that
CRTC in those loops.
>
>>
>>
>>> +
>>> + 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.
>>
Thanks for pointing this out, it seems the problem is a bit more
complicated.
This was done under the assumption that plane states can be validated
without calling dm_update_plane_states() beforehand. It seems that's not
the case.
For the plane disable case, this would actually not alter behavior. The
old sequence sets dm_palne_state->dc_state to NULL in update_planes(),
which would make plane_atomic_check() return early as well (see the 'if'
line below my reply). Now that the sequence is flipped,
plane_atomic_check() would simply return if plane is disabled.
The problematic part is when planes are being enabled. Since
plane_atomic_check() is now called first, the dc_plane_state wouldn't
exist for us to validate. In the old sequence, update_planes() would
have created the dc_plane_state, and added it to the dc context.
It looks like we should be able to create a dc_plane_state without
adding it to the context. dc_validate_plane() (and the hw-specific
implementations called) don't seem to care about anything outside of
that either. I'll dig more into it.
Leo
>>> 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
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
