On 2017-10-12 02:00 AM, Maarten Lankhorst wrote:
> Op 11-10-17 om 22:40 schreef Harry Wentland:
>> On 2017-10-11 03:46 PM, Maarten Lankhorst wrote:
>>> Op 11-10-17 om 20:55 schreef Leo:
>>>>
>>>> On 2017-10-11 10:30 AM, Maarten Lankhorst wrote:
>>>>> Op 11-10-17 om 16:24 schreef sunpeng.li at amd.com:
>>>>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>>>>>
>>>>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>>>>
>>>>>> List of affected functions:
>>>>>>
>>>>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>>>> - Old from_state_var flag was always choosing the new state
>>>>>>
>>>>>> amdgpu_dm_display_resume: use for_each_new
>>>>>> - drm_atomic_helper_duplicate_state is called during suspend to
>>>>>> cache the state
>>>>>> - It sets 'state' within the state triplet to 'new_state'
>>>>>>
>>>>>> amdgpu_dm_commit_planes: use for_each_old
>>>>>> - Called after the state was swapped (via atomic commit tail)
>>>>>>
>>>>>> amdgpu_dm_atomic_commit: use for_each_new
>>>>>> - Called before the state is swapped
>>>>>>
>>>>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>>>> - Called after the state was swapped
>>>>>>
>>>>>> dm_update_crtcs_state: use for_each_new
>>>>>> - Called before the state is swapped (via atomic check)
>>>>>>
>>>>>> amdgpu_dm_atomic_check: use for_each_new
>>>>>> - Called before the state is swapped
>>>>>>
>>>>>> Also fix some typos.
>>>>>>
>>>>>> crct -> crtc
>>>>>> undersacn -> underscan
>>>>>>
>>>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>>>>>> ---
>>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> This patch goes on top of your fixup for new api's patch. Please feel
>>>>>> free to squash them.
>>>>>>
>>>>>> Thanks,
>>>>>> Leo
>>>>>>
>>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++++++++--------------
>>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 5 ++-
>>>>>> 2 files changed, 16 insertions(+), 27 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 9bfe1f9..9394558 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> @@ -568,25 +568,17 @@ static int dm_suspend(void *handle)
>>>>>> return ret;
>>>>>> }
>>>>>> -struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>>>>>> +struct amdgpu_dm_connector *amdgpu_dm_find_first_crtc_matching_connector(
>>>>>> struct drm_atomic_state *state,
>>>>>> - struct drm_crtc *crtc,
>>>>>> - bool from_state_var)
>>>>>> + struct drm_crtc *crtc)
>>>>>> {
>>>>>> uint32_t i;
>>>>>> struct drm_connector_state *conn_state;
>>>>>> struct drm_connector *connector;
>>>>>> struct drm_crtc *crtc_from_state;
>>>>>> - for_each_new_connector_in_state(
>>>>>> - state,
>>>>>> - connector,
>>>>>> - conn_state,
>>>>>> - i) {
>>>>>> - crtc_from_state =
>>>>>> - from_state_var ?
>>>>>> - conn_state->crtc :
>>>>>> - connector->state->crtc;
>>>>>> + for_each_new_connector_in_state(state, connector, conn_state, i) {
>>>>>> + crtc_from_state = conn_state->crtc;
>>>>> Please assign crtc_from_state here with drm_atomic_get_(new/old)_crtc_state.
>>>> We're trying to fetch a drm_crtc from a drm_connector_state, I don't think the state getters are applicable here.
>>>>
>>>> Also, the function should really be named 'find_first_connector_matching_crtc'. I'll fix that.
>>> Oh I misunderstood. But in general derefencing $obj->state should be frowned upon. If you ever want to support atomic flip depth > 1,
>>> all those references should be gone from your driver, and replaced with get_old/new_state..
>> If I understand correctly this is the forward-looking way of how we get the
>> state now? I still see plenty of $obj->state in i915 and other drivers.
>>
>> Any objections to doing this as a follow-up patch?
>>
>> What's atomic flip depth > 1?
>
> That we allow multiple uncompleted nonblocking atomic commits to the same crtc,
> which requires that during commit, $obj->state is not the same as
> new_$obj_state any more. It can be set to an even newer state, but the current
> commit has to set the state that is in state->$obj[i].new_state. This can be
> obtained with either the iterators, or drm_atomic_get_new_$obj_state.
>
> This is why we we got rid of the old iterators, get_existing_state and $obj->state
> were no longer sufficient for this.
>
> Using drm_atomic_get_old/new_obj_state should be a separate patch, but can be fixed
> opportunistically.
>
> But something like
>
> for_each_old_crtc_in_state(...) {
> new_crtc_state = crtc->state
>
> ....
> }
>
> should definitely be fixed in this patch. It's what the iterators are for. :)
>
> I know i915 is still dereferencing $obj->state, but we're trying to slowly fix these
> when we come across them. This usually involves passing the correct state object up
> the callchain, which can be quite deep.
>
> Cheers,
> Maarten
Thanks for the clarification.
We're in a similar situation with some of the deeply nested functions,
most of which don't take in the state object. That said, there are a few
functions lower in the call stack that are eating the correct atomic
state (like the ones you've pointed out). We can target those for now,
and target more when the opportunity comes.
Leo
>
>> Harry
>>
>>>>>
>>>>>> if (crtc_from_state == crtc)
>>>>>> return to_amdgpu_dm_connector(connector);
>>>>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>>>> unsigned long flags;
>>>>>> /* update planes when needed */
>>>>>> - for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>>>>> + for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>>>> struct drm_plane_state *plane_state = plane->state;
>>>>>> struct drm_crtc *crtc = plane_state->crtc;
>>>>>> struct drm_framebuffer *fb = plane_state->fb;
>>>>> Use for_each_oldnew_*_in_state
>>>>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>> dm_state = to_dm_atomic_state(state);
>>>>>> /* update changed items */
>>>>>> - for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>> struct drm_crtc_state *new_state = crtc->state;
>>>>> Same.
>>>> Good points, it seems like there's quite a few places where drm interfaces can be used. Thanks for pointing this out.
>>>>
>>>> Leo
>>>>
>>>>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>> new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>>>>>> new_stream = new_acrtc_state->stream;
>>>>>> - aconnector =
>>>>>> - amdgpu_dm_find_first_crct_matching_connector(
>>>>>> + aconnector = amdgpu_dm_find_first_crtc_matching_connector(
>>>>>> state,
>>>>>> - &new_crtcs[i]->base,
>>>>>> - false);
>>>>>> + &new_crtcs[i]->base);
>>>>>> if (!aconnector) {
>>>>>> DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
>>>>>> "skipping freesync init\n",
>>>>>> @@ -4150,8 +4140,8 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>> }
>>>>>> }
>>>>>> - /* Handle scaling and undersacn changes*/
>>>>>> - for_each_new_connector_in_state(state, connector, old_conn_state, i) {
>>>>>> + /* Handle scaling and underscan changes*/
>>>>>> + for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>>>>>> struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>>>>>> struct dm_connector_state *con_new_state =
>>>>>> to_dm_connector_state(aconnector->base.state);
>>>>> Same here..
>>>>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>> }
>>>>>> /* update planes when needed per crtc*/
>>>>>> - for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>>> + for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>>> new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>>> Same.
>>>>>> if (new_acrtc_state->stream)
>>>>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>> * mark consumed event for drm_atomic_helper_commit_hw_done
>>>>>> */
>>>>>> spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>>>>> - for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>> if (acrtc->base.state->event)
>>>>> Same. You're dereferencing crtc->state
>>>>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>>>> new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>>>> acrtc = to_amdgpu_crtc(crtc);
>>>>>> - aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>>>>> + aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>>>>>> /* TODO This hack should go away */
>>>>>> if (aconnector) {
>>>>>> @@ -4715,7 +4705,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>>>> if (ret)
>>>>>> goto fail;
>>>>>> - /* Check scaling and undersacn changes*/
>>>>>> + /* Check scaling and underscan changes*/
>>>>>> /*TODO Removed scaling changes validation due to inability to commit
>>>>>> * new stream into context w\o causing full reset. Need to
>>>>>> * decide how to handle.
>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>>> index 630e6cd..c1b77bd 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>>> @@ -226,10 +226,9 @@ extern const struct amdgpu_ip_block_version dm_ip_block;
>>>>>> void amdgpu_dm_update_connector_after_detect(
>>>>>> struct amdgpu_dm_connector *aconnector);
>>>>>> -struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>>>>>> +struct amdgpu_dm_connector *amdgpu_dm_find_first_crtc_matching_connector(
>>>>>> struct drm_atomic_state *state,
>>>>>> - struct drm_crtc *crtc,
>>>>>> - bool from_state_var);
>>>>>> + struct drm_crtc *crtc);
>>>>>> struct amdgpu_framebuffer;
>>>>>
>
