From patchwork Thu Jun 2 21:00:54 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lyude Paul X-Patchwork-Id: 12868171 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 20E74C433EF for ; Thu, 2 Jun 2022 21:01:17 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B1F710EEAD; Thu, 2 Jun 2022 21:01:13 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id 44D4610EEAD for ; Thu, 2 Jun 2022 21:01:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1654203670; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=r5MolzJKkbnpxmWaLe/FzIP2Xhwby+9gMYBxVEeA6Pk=; b=TRzrReUBEUO/q3FQd2w/neJdn6+mMbZ7cCkB1GjKJsCfRrnHkfbl/3RTNxK9oIRGAaHq+u UZBdrFYkffiIgxZ/IEyDqz0tG6ckS31zpMUhwQ6BmeCQUWYm1mTVUyFrX/mDQSXe1u281L DixxowpiZ74+AbCMzOSy8+NM0Jjzt7o= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-436-3pXaGPQYPyiBfbwExNO2rw-1; Thu, 02 Jun 2022 17:01:06 -0400 X-MC-Unique: 3pXaGPQYPyiBfbwExNO2rw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0C03080029D; Thu, 2 Jun 2022 21:01:06 +0000 (UTC) Received: from emerald.redhat.com (unknown [10.22.34.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id C4CA7140240A; Thu, 2 Jun 2022 21:01:04 +0000 (UTC) From: Lyude Paul To: amd-gfx@freedesktop.org Subject: [PATCH 1/3] drm/amdgpu/dm/mst: Stop grabbing mst_mgr->lock in compute_mst_dsc_configs_for_state() Date: Thu, 2 Jun 2022 17:00:54 -0400 Message-Id: <20220602210056.73316-2-lyude@redhat.com> In-Reply-To: <20220602210056.73316-1-lyude@redhat.com> References: <20220602210056.73316-1-lyude@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.7 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Zimmermann , Leo Li , open list , "open list:DRM DRIVERS" , "Pan, Xinhui" , Rodrigo Siqueira , Roman Li , Hersen Wu , Nicholas Kazlauskas , David Airlie , Fangzhi Zuo , "open list:AMD DISPLAY CORE" , Alex Deucher , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Noticed this while trying to update amdgpu for the non-atomic MST removal changes - for some reason we appear to grab mst_mgr->lock before computing mst DSC configs. This is wrong though - mst_mgr->lock is only needed while traversing the actual MST topology state - which is not typically something that DRM drivers should be doing themselves anyway. Signed-off-by: Lyude Paul --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 9221b6690a4a..cb3b0e08acc4 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -1056,13 +1056,10 @@ bool compute_mst_dsc_configs_for_state(struct drm_atomic_state *state, if (!is_dsc_need_re_compute(state, dc_state, stream->link)) continue; - mutex_lock(&aconnector->mst_mgr.lock); if (!compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars, &link_vars_start_index)) { - mutex_unlock(&aconnector->mst_mgr.lock); return false; } - mutex_unlock(&aconnector->mst_mgr.lock); for (j = 0; j < dc_state->stream_count; j++) { if (dc_state->streams[j]->link == stream->link) From patchwork Thu Jun 2 21:00:55 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lyude Paul X-Patchwork-Id: 12868173 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 123FAC433EF for ; Thu, 2 Jun 2022 21:01:22 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2678A112985; Thu, 2 Jun 2022 21:01:16 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5457F10E427 for ; Thu, 2 Jun 2022 21:01:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1654203671; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oqtjLES5h9VyqujFfP0LIXi0+7ZX2bu0LxgK1iAi69Q=; b=U9avYneDIlBG87XTRH4qQN5K4J2pAN078eK5WNsYk+dnhQzixJEWfP3t2vv+XFc2KYg9HC H7tuIVZfeQoIPHWzNNIM4ukrj9wCqciiDPUB59PLDdDwRy92Lv7SDPRM8QCb0zuxVM4adK QwJNxZdzKVOq1TVkwG4SvU2d+NYjK5E= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-629-niFFMZ9nMa2Ed0rJvorMbQ-1; Thu, 02 Jun 2022 17:01:08 -0400 X-MC-Unique: niFFMZ9nMa2Ed0rJvorMbQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 609E8101A54E; Thu, 2 Jun 2022 21:01:07 +0000 (UTC) Received: from emerald.redhat.com (unknown [10.22.34.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id BD5671410F36; Thu, 2 Jun 2022 21:01:06 +0000 (UTC) From: Lyude Paul To: amd-gfx@freedesktop.org Subject: [PATCH 2/3] drm/amdgpu/dm/mst: Stop grabbing mst_mgr->lock in pre_compute_mst_dsc_configs_for_state() Date: Thu, 2 Jun 2022 17:00:55 -0400 Message-Id: <20220602210056.73316-3-lyude@redhat.com> In-Reply-To: <20220602210056.73316-1-lyude@redhat.com> References: <20220602210056.73316-1-lyude@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.7 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Zimmermann , Leo Li , open list , "open list:DRM DRIVERS" , "Pan, Xinhui" , Rodrigo Siqueira , Roman Li , Hersen Wu , Nicholas Kazlauskas , David Airlie , "open list:AMD DISPLAY CORE" , Alex Deucher , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" This lock is only needed if you're iterating through the in-memory topology (e.g. drm_dp_mst_branch->ports, drm_dp_mst_port->mstb, etc.). This doesn't actually seem to be what's going on here though, so we can just drop this lock. Signed-off-by: Lyude Paul --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index cb3b0e08acc4..1259f2f7a8f9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -1112,16 +1112,12 @@ static bool if (!is_dsc_need_re_compute(state, dc_state, stream->link)) continue; - mutex_lock(&aconnector->mst_mgr.lock); if (!compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars, - &link_vars_start_index)) { - mutex_unlock(&aconnector->mst_mgr.lock); + &link_vars_start_index)) return false; - } - mutex_unlock(&aconnector->mst_mgr.lock); for (j = 0; j < dc_state->stream_count; j++) { if (dc_state->streams[j]->link == stream->link) From patchwork Thu Jun 2 21:00:56 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Lyude Paul X-Patchwork-Id: 12868172 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E767FC433EF for ; Thu, 2 Jun 2022 21:01:19 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E2D891122E7; Thu, 2 Jun 2022 21:01:13 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id 62FDB10F94D for ; Thu, 2 Jun 2022 21:01:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1654203671; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=krZbVlK3Z1/IgWXud2lUTsTQaV8i8IJv68G2uGF/JVY=; b=Nk/xoewyXr2BLsx2HlTov3XvRYkqe3fSUtJN8pj5UNVkG7i8/tP49hweeX/gBM7QnAlZOR aP6zASGqbkGrr+e8T5UFFPYboaO+wrJpsj3tZMMJq3MdWezHkwN8owh6z4nexh/mhzqJsL i5eswOb1lF5UyOBXIAbE21o9wS5dKd0= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-55-dLxcq075PHG6Mmz-Sz_acg-1; Thu, 02 Jun 2022 17:01:09 -0400 X-MC-Unique: dLxcq075PHG6Mmz-Sz_acg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B739F3C10142; Thu, 2 Jun 2022 21:01:08 +0000 (UTC) Received: from emerald.redhat.com (unknown [10.22.34.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1E6821410F36; Thu, 2 Jun 2022 21:01:08 +0000 (UTC) From: Lyude Paul To: amd-gfx@freedesktop.org Subject: [PATCH 3/3] drm/amdgpu/dm: Drop != NULL check in dm_mst_get_pbn_divider() Date: Thu, 2 Jun 2022 17:00:56 -0400 Message-Id: <20220602210056.73316-4-lyude@redhat.com> In-Reply-To: <20220602210056.73316-1-lyude@redhat.com> References: <20220602210056.73316-1-lyude@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.7 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Zimmermann , Leo Li , open list , "open list:DRM DRIVERS" , "Pan, Xinhui" , Rodrigo Siqueira , Roman Li , Hersen Wu , Nicholas Kazlauskas , David Airlie , Fangzhi Zuo , "open list:AMD DISPLAY CORE" , Alex Deucher , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" A lot of code in amdgpu seems to sprinkle in if (foo != NULL) … Checks pretty much all over the place, many times in locations where it's clear foo (whatever foo may be) should never be NULL unless we've run into a programming error. This is definitely one of those places, as dm_mst_get_pbn_divider() should never be getting called with a NULL dc_link pointer. The problem with this code pattern is that many times the places I've seen it used in amdgpu have no real error handling. This is actually quite bad, if we try to avoid the NULL pointer and instead simply skip any code that was expecting a valid pointer - we're already in undefined territory. Subsequent code we execute may have expected sideaffects from the code we skipped that are no longer present, which leads to even more unpredictable behavior then a simple segfault. This could be silent errors or even just another segfault somewhere else. If we simply segfault though, that's not good either. But unlike the former solution, no subsequent code in the kernel thread will execute - and we will likely even get a clear backtrace from the invalid memory access. Of course, the preferred approach is to simply handle the possibility of both NULL and non-NULL pointers with nice error handling code. However, that's not always desirable or even possible, and in those cases it's likely just better to fail predictably rather than unpredictably. This code is a nice example of that - if link is NULL, you'll return a PBN divisor of 0. And thus, you've simply traded in your potential segfault for a potential divide by 0 error. This was something I actually managed to hit while working on the legacy MST removal work. Signed-off-by: Lyude Paul --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 1259f2f7a8f9..35c7def8f2bd 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -537,9 +537,6 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm, int dm_mst_get_pbn_divider(struct dc_link *link) { - if (!link) - return 0; - return dc_link_bandwidth_kbps(link, dc_link_get_link_cap(link)) / (8 * 1000 * 54); }