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 14:24:01 +0100 Message-ID: <536B8571.20101@linux.intel.com> References: <1398698528-25256-1-git-send-email-bradley.d.volkin@intel.com> <536B54B5.4010904@linux.intel.com> <20140508114457.GB20257@strange.amr.corp.intel.com> <536B77BD.6030508@linux.intel.com> <20140508130211.GA22477@strange.amr.corp.intel.com> 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 56F3F6EE5B for ; Thu, 8 May 2014 06:24:41 -0700 (PDT) In-Reply-To: <20140508130211.GA22477@strange.amr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 05/08/2014 02:02 PM, Damien Lespiau wrote: > On Thu, May 08, 2014 at 01:25:33PM +0100, Tvrtko Ursulin wrote: >> >> On 05/08/2014 12:44 PM, Damien Lespiau wrote: >>> On Thu, May 08, 2014 at 10:56:05AM +0100, 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? >>> >>> Those number_of_cmds allocations are a bit awkward though, couldn't we >>> just embed the hlist_node into the desciptor struct? >> >> Until Brad comes online, I think that is because command descriptors >> to hash table entries are not 1-to-1. > > Ah, I guess the common cmds are part of several hash tables. We could at > least turn the multiple allocations into one big allocation though. Probably a problem since commands can be added dynamically. Also from the memory use point of view, single allocations are 12/24 bytes so fit into 16/32 byte slabs. That's some wastage on the face of it, but bunching them up would make... say common plus render commands are ~30 in total, so 360/720 bytes. Which would waste 152/304 bytes (512 and 1024 slabs) vs. 120/240 bytes for individual allocations. So for 30 command array current approach is even better. :) Maybe make a dedicated cache for struct cmd_node? Tvrtko