From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/53] drm/i915/bdw: Introduce one context backing object per engine Date: Wed, 18 Jun 2014 22:16:24 +0200 Message-ID: <20140618201624.GW5821@phenom.ffwll.local> References: <1402673891-14618-1-git-send-email-oscar.mateo@intel.com> <1402673891-14618-7-git-send-email-oscar.mateo@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f174.google.com (mail-wi0-f174.google.com [209.85.212.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 0D0336E785 for ; Wed, 18 Jun 2014 13:16:31 -0700 (PDT) Received: by mail-wi0-f174.google.com with SMTP id bs8so8293079wib.1 for ; Wed, 18 Jun 2014 13:16:31 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402673891-14618-7-git-send-email-oscar.mateo@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: oscar.mateo@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 13, 2014 at 04:37:24PM +0100, oscar.mateo@intel.com wrote: > From: Oscar Mateo > > A context backing object only makes sense for a given engine (because > it holds state data specific to that engine). > > In legacy ringbuffer sumission mode, the only MI_SET_CONTEXT we really > perform is for the render engine, so one backing object is all we needed. > > With Execlists, however, we need backing objects for every engine, as > contexts become the only way to submit workloads to the GPU. To tackle > this problem, we multiplex the context struct to contain > objects. > > Originally, I colored this code by instantiating one new context for > every engine I wanted to use, but this change suggested by Brad Volkin > makes it more elegant. > > v2: Leave the old backing object pointer behind. Daniel Vetter suggested > using a union, but it makes more sense to keep render_obj as a NULL > pointer behind, to make sure no one uses it incorrectly when Execlists > are enabled, similar to what we are doing with ring->buffer (Rusty's API > level 5). > > Signed-off-by: Oscar Mateo > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a15370c..ccc1ba6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -593,7 +593,14 @@ struct intel_context { > uint8_t remap_slice; > struct drm_i915_file_private *file_priv; > struct intel_engine_cs *last_ring; > + > + /* Legacy ring buffer submission */ > struct drm_i915_gem_object *render_obj; Per my previous request, is_initialized should also be nearby, maybe wrapped in a struct. So union { struct { struct gem_bo *obj; bool is_iniatlized; } render_ctx; struct { ... } lrc[I915_NUM_RINGS]; } Or some other means to make it clearer which fields are for legacy render ctx objects and which for lrc contexts. I also wonder whether we should shovel all the hw specific stuff at the end to have a clearer separation between the sw-side field members associated with the software context object and the stuff for the hw thing. Just ideas to pick&choose really, we can cocci-polish this once it's all settled easily (i.e. afterwards). -Daniel > + /* Execlists */ > + struct { > + struct drm_i915_gem_object *obj; > + } engine[I915_NUM_RINGS]; > + > struct i915_ctx_hang_stats hang_stats; > struct i915_address_space *vm; > > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch