On 12/19/2018 08:54 AM, Kazlauskas, Nicholas wrote:
> On 12/18/18 3:12 PM, Grodzovsky, Andrey wrote:
>>
>> On 12/18/2018 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)
>> What if DRM submits a request for plane update without any CRTC in the
>> new atomic state - like some type of plane update request e.g. disable
>> plane) - Can this approach handle this scenario ?
>>
>> Andrey
> I don't think dm_update_planes_state will end up getting called if the
> CRTC wasn't in the commit (since it would skip the atomic check for the
> CRTC).
That what I meant in a form of a question :) Unless proven wrong I don't
think it's a
correct approach to tie plane validations into crtc validations in
general since it creates
an extra constraint which is actually contrary to the atomic modeset
design where CRTCs and planes
are independent entities. What we have today with doing all the
validation in admgpu_dm_atomic_check
is for sure not the most correct approach but again, that because of DC
constraints and the best way to go
to align with DRM is to tackle the DC design.
>
> Ideally we would be calling dm_update_planes_state directly in the plane
> atomic check instead of doing it through the CRTC atomic check.
>
> Like you said in your other email, it would be better if we could skip
> the add/remove in reverse order aspect.
>
> It might be worth looking into fixing dc to be able to do that if it
> doesn't currently support it.
I can't remember the exact limitation for DC why not break apart the
dependency dc_plane has on dc_stream but I am sure
Dmitry or Tony can tell you - you can start from that.
Andrey
>
> Nicholas Kazlauskas
>
>>> 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;
>>> +
>>> + 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;
>>> +
>>> 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 */
>>> ret = drm_atomic_helper_check_planes(dev, state);
>>> if (ret)
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
