From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH] drm/i915: Use hash tables for the command parser Date: Thu, 08 May 2014 16:50:40 +0100 Message-ID: <536BA7D0.7080803@linux.intel.com> References: <1398698528-25256-1-git-send-email-bradley.d.volkin@intel.com> <536B54B5.4010904@linux.intel.com> <20140508152716.GA21594@bdvolkin-ubuntu-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id EDF5A6E6A8 for ; Thu, 8 May 2014 08:51:18 -0700 (PDT) In-Reply-To: <20140508152716.GA21594@bdvolkin-ubuntu-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Volkin, Bradley D" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On 05/08/2014 04:27 PM, Volkin, Bradley D wrote: > On Thu, May 08, 2014 at 02:56:05AM -0700, Tvrtko Ursulin wrote: >> >> Hi Brad, >> >> On 04/28/2014 04:22 PM, bradley.d.volkin@intel.com wrote: >> [snip] >>> - BUG_ON(!validate_cmds_sorted(ring)); >>> + BUG_ON(!validate_cmds_sorted(ring, cmd_tables, cmd_table_count)); >>> BUG_ON(!validate_regs_sorted(ring)); >>> + >>> + BUG_ON(init_hash_table(ring, cmd_tables, cmd_table_count)); >> >> Is a BUG_ON a bit harsh since the above fails only on ENOMEM condition? >> >> If the concern is not allowing any command execution if parser setup has >> failed, it would be nicer to the system as whole to just keep rejecting >> everything, but let the rest of the kernel live to enable debug or whatever? >> >> I know it won't happen almost ever so it's a minor point really. I just >> dislike actively hosing the whole system if it is avoidable. > > Hi Tvrtko, > > I agree that a BUG_ON might be harsh here. I suppose we could log an > error and disable the command parser. Most command buffers would > still go through fine but HW parsing would reject some that the SW > parser might otherwise allow. That could be a bit odd if we ever did > get a failure - apps/functionality that worked the last time I booted > suddenly don't this time. The issue would be in the log though. That would indeed be weird. So command parser is just a more permissive whitelist, a super set of hardware parser? > I don't have a strong preference on this. Whatever people prefer. Initially I was thinking of just rejecting all batch buffers if such fundamental initialisation fails. It beats BUG_ON in the sense that it leaves the system alive and it doesn't suffer from the random subtle behaviour like if it would just disable the command parser. It is a bit theoretical because it is likely system won't be too healthy anyway if such a tiny allocation fails but you never know. It's better not to crash it if we don't have to. Regards, Tvrtko