From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Revert "drm/amd/pm: Disable SCLK switching on Oland with high pixel clocks (v3)" Date: Mon, 23 Feb 2026 06:01:57 +1000 Message-ID: In-Reply-To: <20260221034402.69537-3-rosenp@gmail.com> References: <20260221034402.69537-1-rosenp@gmail.com> <20260221034402.69537-3-rosenp@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The commit message has a reasonable explanation but has a few issues: > In 6.17, threre are further commits that are needed to get the DC > codepath in amdgpu for Southern Islands GPUs working but they seem to be > too much of a hastle to backport cleanly. The simplest solution is to > revert this problematic commit Typos: "threre" should be "there", "hastle" should be "hassle". The last paragraph is also missing a period. Per the mailing list discussion, the commit message should also specify which stable kernels are affected (6.1.y, 6.6.y, 6.12.y) and clarify that 6.17+ already has proper fixes. The code change removes the entire high-pixelclock counting loop and its associated logic: > - for (i = 0; i < adev->mode_info.num_crtc; i++) { > - if (!(adev->pm.dpm.new_active_crtcs & (1 << i)) || > - !adev->mode_info.crtcs[i]->enabled) > - continue; > - > - conn = to_amdgpu_connector(adev->mode_info.crtcs[i]->connector); > - > - if (conn->pixelclock_for_modeset > 297000) > - high_pixelclock_count++; > - } This is the code that causes the panic -- the `to_amdgpu_connector()` call on `adev->mode_info.crtcs[i]->connector` likely hits a NULL or invalid pointer when the DC codepath is active on SI GPUs. Removing it fixes the crash. The patch also cleans up the now-unused variable declarations: > - struct amdgpu_connector *conn; > ... > - u32 high_pixelclock_count = 0; `disable_sclk_switching` remains declared and initialized to `false` but is never set to `true` after both patches. This is correct behavior -- the variable existed before the original commit and is referenced later in the function, so it must stay. It will just always be `false`, effectively disabling the SCLK switching workaround. No correctness issues with the code change. The tradeoff (losing the flickering workaround vs. fixing a crash) is clearly the right one for stable kernels. --- Generated by Claude Code Patch Reviewer