From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikola Pajkovsky Subject: Re: [RFC] drm/amd/amdgpu: get rid of else branch Date: Thu, 04 May 2017 14:57:26 +0200 Message-ID: <87efw47pah.fsf@suse.cz> References: <6ce3044b-9473-94eb-e54c-e8e856f1c88f@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <6ce3044b-9473-94eb-e54c-e8e856f1c88f@amd.com> ("Christian =?utf-8?Q?K=C3=B6nig=22's?= message of "Fri, 28 Apr 2017 10:30:49 +0200") Sender: linux-kernel-owner@vger.kernel.org To: Christian =?utf-8?Q?K=C3=B6nig?= Cc: linux-kernel@vger.kernel.org, Alex Deucher , David Airlie , dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org List-Id: amd-gfx.lists.freedesktop.org Christian König writes: > Am 27.04.2017 um 18:17 schrieb Nikola Pajkovsky: >> This is super simple elimination of else branch and I should >> probably even use unlikely in >> >> if (ring->count_dw < count_dw) { >> >> However, amdgpu_ring_write() has similar if condition, but does not >> return after DRM_ERROR and it looks suspicious. On error, we still >> adding v to ring and keeping count_dw-- below zero. >> >> if (ring->count_dw <= 0) >> DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n"); >> ring->ring[ring->wptr++] = v; >> ring->wptr &= ring->ptr_mask; >> ring->count_dw--; >> >> I can obviously be totaly wrong. Hmm? > > That's just choosing the lesser evil. > > When we write more DW to the ring than expected it is possible (but > not likely) that we override stuff on the ring buffer which is still > executed by the command processor leading to a possible CP crash. > > But when we completely drop the write the commands in the ring buffer > will certainly be invalid and so the CP will certainly crash sooner or > later. Instead of choosing the lesser evil, is there good way to design ring buffer right way? > Please add the unlikely() as well and then send out the patch with a > signed-of-by line and I will be happy to push it into our upstream > branch. Proper patch has been sent. -- Nikola From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754405AbdEDM5q convert rfc822-to-8bit (ORCPT ); Thu, 4 May 2017 08:57:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:59121 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751211AbdEDM53 (ORCPT ); Thu, 4 May 2017 08:57:29 -0400 From: Nikola Pajkovsky To: Christian =?utf-8?Q?K=C3=B6nig?= Cc: , Alex Deucher , David Airlie , , Subject: Re: [RFC] drm/amd/amdgpu: get rid of else branch References: <6ce3044b-9473-94eb-e54c-e8e856f1c88f@amd.com> Date: Thu, 04 May 2017 14:57:26 +0200 In-Reply-To: <6ce3044b-9473-94eb-e54c-e8e856f1c88f@amd.com> ("Christian =?utf-8?Q?K=C3=B6nig=22's?= message of "Fri, 28 Apr 2017 10:30:49 +0200") Message-ID: <87efw47pah.fsf@suse.cz> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Christian König writes: > Am 27.04.2017 um 18:17 schrieb Nikola Pajkovsky: >> This is super simple elimination of else branch and I should >> probably even use unlikely in >> >> if (ring->count_dw < count_dw) { >> >> However, amdgpu_ring_write() has similar if condition, but does not >> return after DRM_ERROR and it looks suspicious. On error, we still >> adding v to ring and keeping count_dw-- below zero. >> >> if (ring->count_dw <= 0) >> DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n"); >> ring->ring[ring->wptr++] = v; >> ring->wptr &= ring->ptr_mask; >> ring->count_dw--; >> >> I can obviously be totaly wrong. Hmm? > > That's just choosing the lesser evil. > > When we write more DW to the ring than expected it is possible (but > not likely) that we override stuff on the ring buffer which is still > executed by the command processor leading to a possible CP crash. > > But when we completely drop the write the commands in the ring buffer > will certainly be invalid and so the CP will certainly crash sooner or > later. Instead of choosing the lesser evil, is there good way to design ring buffer right way? > Please add the unlikely() as well and then send out the patch with a > signed-of-by line and I will be happy to push it into our upstream > branch. Proper patch has been sent. -- Nikola