[PATCH 56/73] drm/amd/display: Remove dangling planes on dc commit state

[Andrey Grodzovsky]

Ok, got it.

Thanks,
Andrey

On 11/14/2017 10:10 AM, Cheng, Tony wrote:
> DC always work off current context.  If we don't swap in a context with plane removed, HWSS will be doing the wrong programming when we enable streams and planes.
>
> -----Original Message-----
> From: Grodzovsky, Andrey
> Sent: Tuesday, November 14, 2017 10:08 AM
> To: Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>; amd-gfx at lists.freedesktop.org; Cheng, Tony <Tony.Cheng at amd.com>; Sun, Yongqiang <Yongqiang.Sun at amd.com>
> Subject: Re: [PATCH 56/73] drm/amd/display: Remove dangling planes on dc commit state
>
>
>
> On 11/13/2017 04:53 PM, Leo Li wrote:
>>
>> On 2017-11-10 02:00 PM, Andrey Grodzovsky wrote:
>>>
>>> On 11/09/2017 03:05 PM, Harry Wentland wrote:
>>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>>>
>>>> When disabling pipe splitting, we need to make sure we disable both
>>>> planes used.
>>>>
>>>> This should be done for Linux as well.
>>>>
>>>> Change-Id: I79f5416a55bd26c19ca3cfb346a943d69872a8ce
>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>>>> Reviewed-by: Tony Cheng <Tony.Cheng at amd.com>
>>>> Acked-by: Harry Wentland <harry.wentland at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/dc/core/dc.c | 39
>>>> ++++++++++++++++++++++++++++----
>>>>    1 file changed, 35 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
>>>> b/drivers/gpu/drm/amd/display/dc/core/dc.c
>>>> index 56df1304e49c..d70dbc102123 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
>>>> @@ -629,6 +629,39 @@ static bool construct(struct dc *dc,
>>>>        return false;
>>>>    }
>>>> +static void disable_dangling_plane(struct dc *dc, struct dc_state
>>>> *context)
>>>> +{
>>>> +    int i, j;
>>>> +    struct dc_state *dangling_context = dc_create_state();
>>>> +    struct dc_state *current_ctx;
>>>> +
>>>> +    if (dangling_context == NULL)
>>>> +        return;
>>>> +
>>>> +    dc_resource_state_copy_construct(dc->current_state,
>>>> dangling_context);
>>>> +
>>>> +    for (i = 0; i < dc->res_pool->pipe_count; i++) {
>>>> +        struct dc_stream_state *old_stream =
>>>> + dc->current_state->res_ctx.pipe_ctx[i].stream;
>>>> +        bool should_disable = true;
>>>> +
>>>> +        for (j = 0; j < context->stream_count; j++) {
>>>> +            if (old_stream == context->streams[j]) {
>>>> +                should_disable = false;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +        if (should_disable && old_stream) {
>>>> +            dc_rem_all_planes_for_stream(dc, old_stream,
>>>> dangling_context);
>>> Why this is not happening in atomic_check during
>>> dm_update_planes_state with enable set to false ? Since the old
>>> stream is present I assume old crtc_state for planes to disable is
>>> present and as I see from the code it should happen in that function
>>>
>>> Thanks,
>>> Andrey
>> You're correct, the above logic should be done in atomic check.
>> However, the apply_ctx_for_surface call below is the reason why this
>> code (or rather, this entire function) is here.
>>
>> Due to some refactoring efforts that rearranged the front-end and
>> back-end programming sequence, the logic for disabling surfaces for
>> streams have been moved here. disable_dangling_planes is responsible
>> for disabling the front end of streams that do not exist in the new
>> dc_state (i.e. streams that are disabled). See one of the relevant patches here:
>> https://lists.freedesktop.org/archives/amd-gfx/2017-October/015201.htm
>> l
>>
>> This cannot be done in atomic check, because there is currently no
>> support in DC to explicitly mark a stream or surface as disabled. We
>> infer it by comparing the current and new dc_states, then call
>> apply_ctx to program it.
>>
>> Leo
> I see, at least try to get rid of dangling_context I don't see why intermediate state is needed. Adding Tony and Jonathan, maybe they can explain.
> Also please rename  struct dc_state *context in the argument list to state, context is obsolete naming.
>
> Thanks,
> Andrey
>
>>>> + dc->hwss.apply_ctx_for_surface(dc, old_stream, 0,
>>>> + dc->dangling_context);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    current_ctx = dc->current_state;
>>>> +    dc->current_state = dangling_context;
>>>> +    dc_release_state(current_ctx);
>>>> +}
>>>> +
>>>> /*******************************************************************
>>>> ************
>>>>
>>>>     * Public functions
>>>> ********************************************************************
>>>> **********/
>>>>
>>>> @@ -833,14 +866,14 @@ static enum dc_status
>>>> dc_commit_state_no_check(struct dc *dc, struct dc_state *c
>>>>        int i, k, l;
>>>>        struct dc_stream_state *dc_streams[MAX_STREAMS] = {0};
>>>> +    disable_dangling_plane(dc, context);
>>>> +
>>>>        for (i = 0; i < context->stream_count; i++)
>>>>            dc_streams[i] =  context->streams[i];
>>>>        if (!dcb->funcs->is_accelerated_mode(dcb))
>>>>            dc->hwss.enable_accelerated_mode(dc);
>>>> -
>>>> -
>>>>        for (i = 0; i < context->stream_count; i++) {
>>>>            const struct dc_sink *sink = context->streams[i]->sink; @@
>>>> -864,8 +897,6 @@ static enum dc_status
>>>> dc_commit_state_no_check(struct dc *dc, struct dc_state *c
>>>>                }
>>>>            }
>>>> -
>>>> -
>>>>            CONN_MSG_MODE(sink->link, "{%dx%d, %dx%d@%dKhz}",
>>>> context->streams[i]->timing.h_addressable,
>>>> context->streams[i]->timing.v_addressable,