[PATCH v4 2/3] drm/amd/display: Destroy DC context while keeping DML

[Mario Limonciello]

On 10/5/2023 09:27, Alex Deucher wrote:
> On Wed, Oct 4, 2023 at 1:37 PM Mario Limonciello
> <mario.limonciello at amd.com> wrote:
>>
>> If there is memory pressure at suspend time then dynamically
>> allocating a large structure as part of DC suspend code will
>> fail.
>>
>> Instead re-use the same structure and clear all members except
>> those that should be maintained.
>>
>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2362
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/core/dc.c      | 25 -------------------
>>   .../gpu/drm/amd/display/dc/core/dc_resource.c | 12 +++++++++
>>   2 files changed, 12 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
>> index 39e291a467e2..cb8c7c5a8807 100644
>> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
>> @@ -4728,9 +4728,6 @@ bool dc_set_power_state(
>>          struct dc *dc,
>>          enum dc_acpi_cm_power_state power_state)
>>   {
>> -       struct kref refcount;
>> -       struct display_mode_lib *dml;
>> -
>>          if (!dc->current_state)
>>                  return true;
>>
>> @@ -4750,30 +4747,8 @@ bool dc_set_power_state(
>>                  break;
>>          default:
>>                  ASSERT(dc->current_state->stream_count == 0);
>> -               /* Zero out the current context so that on resume we start with
>> -                * clean state, and dc hw programming optimizations will not
>> -                * cause any trouble.
>> -                */
>> -               dml = kzalloc(sizeof(struct display_mode_lib),
>> -                               GFP_KERNEL);
>> -
>> -               ASSERT(dml);
>> -               if (!dml)
>> -                       return false;
>> -
>> -               /* Preserve refcount */
>> -               refcount = dc->current_state->refcount;
>> -               /* Preserve display mode lib */
>> -               memcpy(dml, &dc->current_state->bw_ctx.dml, sizeof(struct display_mode_lib));
>>
>>                  dc_resource_state_destruct(dc->current_state);
>> -               memset(dc->current_state, 0,
>> -                               sizeof(*dc->current_state));
>> -
>> -               dc->current_state->refcount = refcount;
>> -               dc->current_state->bw_ctx.dml = *dml;
> 
> The dml dance seems a bit weird.  I guess it's here because
> dc_resource_state_destruct() might change it?  Can we safely drop
> this?  If we do need it, we could pre-allocate a dml structure and use
> that.

The dml structure is huge, so I think it's sub-optimal to have two 
copies of it.  That's why I aimed to just destroy everything else except 
it instead.

The only reason it's "safe" to drop the whole above stuff is because of 
"threading the needle" of what dc_resource_state_destruct() does.

In the earlier version I had a mistake to miss clearing the scratch 
variable and it caused some IGT faliures.

This probably needs to be double checked with the DML2 series landing as 
well to make sure it didn't get caught in the middle.

> 
> Alex
> 
>> -
>> -               kfree(dml);
>>
>>                  break;
>>          }
>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> index aa7b5db83644..e487c966c118 100644
>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
>> @@ -4350,6 +4350,18 @@ void dc_resource_state_destruct(struct dc_state *context)
>>                  context->streams[i] = NULL;
>>          }
>>          context->stream_count = 0;
>> +       context->stream_mask = 0;
>> +       memset(&context->res_ctx, 0, sizeof(context->res_ctx));
>> +       memset(&context->pp_display_cfg, 0, sizeof(context->pp_display_cfg));
>> +       memset(&context->dcn_bw_vars, 0, sizeof(context->dcn_bw_vars));
>> +       context->clk_mgr = NULL;
>> +       memset(&context->bw_ctx.bw, 0, sizeof(context->bw_ctx.bw));
>> +       memset(context->block_sequence, 0, sizeof(context->block_sequence));
>> +       context->block_sequence_steps = 0;
>> +       memset(context->dc_dmub_cmd, 0, sizeof(context->dc_dmub_cmd));
>> +       context->dmub_cmd_count = 0;
>> +       memset(&context->perf_params, 0, sizeof(context->perf_params));
>> +       memset(&context->scratch, 0, sizeof(context->scratch));
>>   }
>>
>>   void dc_resource_state_copy_construct(
>> --
>> 2.34.1
>>