From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 09/53] drm/i915/bdw: Initialization for Logical Ring Contexts Date: Thu, 19 Jun 2014 12:08:16 +0200 Message-ID: References: <1402673891-14618-1-git-send-email-oscar.mateo@intel.com> <1402673891-14618-10-git-send-email-oscar.mateo@intel.com> <20140618202436.GZ5821@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A301320868@IRSMSX103.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ig0-f178.google.com (mail-ig0-f178.google.com [209.85.213.178]) by gabe.freedesktop.org (Postfix) with ESMTP id 5E81F6E0D7 for ; Thu, 19 Jun 2014 03:08:17 -0700 (PDT) Received: by mail-ig0-f178.google.com with SMTP id hn18so1792521igb.17 for ; Thu, 19 Jun 2014 03:08:16 -0700 (PDT) In-Reply-To: <92648605EABDA246B775AAB04C95A7A301320868@IRSMSX103.ger.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: "Mateo Lozano, Oscar" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Thu, Jun 19, 2014 at 11:23 AM, Mateo Lozano, Oscar wrote: >> > -static bool hw_context_enabled(struct drm_device *dev) >> > +static bool contexts_enabled(struct drm_device *dev) >> > { >> > - return to_i915(dev)->hw_context_size; >> > + struct drm_i915_private *dev_priv = to_i915(dev); >> > + >> > + /* FIXME: this would be cleaner with a "context type" enum */ >> > + return dev_priv->lrc_enabled || dev_priv->hw_context_size; >> >> Since you have a bunch of if ladders the usual approach isn't an enum but a >> vfunc table to abstract behaviour. Think object types instead of switch >> statements. Style bikeshed though (presume code later on doesn't have >> excesses here). >> -Daniel > > Hmmmm... I offered to do this with vfuncs early on, but you mentioned special-casing should be enough. And I agreed: at the end, the LR contexts are not that different from traditional HW contexts. This is what I had in mind: > > CTX_TYPE_FAKE: no backing objects. > CTX_TYPE_HW: one render backing object at creation time. > CTX_TYPE_LR: n backing objects, with deferred creation. A few functions are moot (e.g. switch, reset). > > The current system (looking at dev_priv->hw_context_size to distinguish fake from hw contexts) is imo a bit obfuscated. > And we can always abstract this away with vfuncs if it becomes too complex in the future... > > What do you think? can special casing made do for the time being? Yeah I generally promote the rule-of-thumb to only do vfuncs once we have the 3rd case (and I don't think we should count fake contexts really). Until then if-ladders and hacks are good enough. Actually better since usually you need a few completely different platforms to know what's required of your vfunc intefaces to cover it all. I really only latched onto this because of your FIXME comment, no other reason at all. So if we decide that some reorg helps the code a lot we can do that as a follow-up, but really not required upfront imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch