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).
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.
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
