[PATCH] drm/amd/display: turn DPMS off on mst connector unplug

[Aurabindo Pillai]

On 2020-11-26 3:11 p.m., Kazlauskas, Nicholas wrote:
> On 2020-11-26 2:50 p.m., Aurabindo Pillai wrote:
>> [Why&How]
>>
>> Set dpms off on the MST connector that was unplugged, for the side
>> effect of
>> releasing some references held through deallocation of mst payload.
>>
>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
>> Signed-off-by: Eryk Brol <eryk.brol at amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++++++++++++++++++-
>>   1 file changed, 62 insertions(+), 1 deletion(-)
>>
>> 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 e213246e3f04..fc984cf6e316 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct
>> dc_state *dc_state,
>>       return;
>>   }
>>   +static void dm_set_dpms_off(struct dc_link *link)
>> +{
>> +    struct {
>> +        struct dc_surface_update surface_updates[MAX_SURFACES];
>> +        struct dc_stream_update stream_update;
>> +    } * bundle;
>> +    struct dc_stream_state *stream_state;
>> +    struct dc_context *dc_ctx = link->ctx;
>> +    int i;
>> +
>> +    bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
>> +
>> +    if (!bundle) {
>> +        dm_error("Failed to allocate update bundle\n");
>> +        return;
>> +    }
>> +
>> +    bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL);
> 
> You don't need to allocate memory for the bool. You can just use a local
> here, DC doesn't need it after the call ends.
> 
> I think the same should apply to the surface updates as well. I'm not
> entirely sure how much stack the bundle takes up for a single stream
> here but it should be small enough that we can leave it on the stack.
> 
>> +
>> +    if (!bundle->stream_update.dpms_off) {
>> +        dm_error("Failed to allocate update bundle\n");
>> +        goto cleanup;
>> +    }
>> +
>> +    *bundle->stream_update.dpms_off = true;
>> +
>> +    for (i = 0; i< MAX_PIPES; i++) {
>> +        if (dc_ctx->dc->current_state->streams[i] == NULL)
>> +            continue;
>> +
>> +        if (dc_ctx->dc->current_state->streams[i]->link == link) {
>> +            stream_state = dc_ctx->dc->current_state->streams[i];
>> +            goto link_found;
>> +        }
>> +    }
> 
> We shouldn't be reading from dc->current_state directly in DM, it's
> essentially private state.
> 
> I think we should actually have a new helper here in dc_stream.h that's
> like:
> 
> struct dc_stream_state *dc_stream_find_from_link(const struct dc_link
> *link);
> 
> to replace this block of code.
> 
> Note that any time we touch current_state we also need to be locking -
> it looks like this function is missing the appropriate calls to:
> 
> mutex_lock(&adev->dm.dc_lock);
> mutex_unlock(&adev->dm.dc_lock);
> 
> Regards,
> Nicholas Kazlauskas
> 
> 

Thank you for the review. Sent a v2