Re: [PATCH 5/5] drm/amd/display: Move the dm update dance to crtc->atomic_check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux