From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer Date: Wed, 13 Aug 2014 17:16:16 +0200 Message-ID: <20140813151616.GD10500@phenom.ffwll.local> References: <1406217891-8912-1-git-send-email-thomas.daniel@intel.com> <1406217891-8912-9-git-send-email-thomas.daniel@intel.com> <20140811141413.GE10500@phenom.ffwll.local> <20140811142032.GF10500@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f173.google.com (mail-we0-f173.google.com [74.125.82.173]) by gabe.freedesktop.org (Postfix) with ESMTP id 82E056E597 for ; Wed, 13 Aug 2014 08:16:05 -0700 (PDT) Received: by mail-we0-f173.google.com with SMTP id q58so11571730wes.18 for ; Wed, 13 Aug 2014 08:16:04 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Daniel, Thomas" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Wed, Aug 13, 2014 at 01:34:15PM +0000, Daniel, Thomas wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Monday, August 11, 2014 3:21 PM > > To: Daniel, Thomas > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an > > engine pointers to the ringbuffer > > > > On Mon, Aug 11, 2014 at 04:14:13PM +0200, Daniel Vetter wrote: > > > On Thu, Jul 24, 2014 at 05:04:16PM +0100, Thomas Daniel wrote: > > > > From: Oscar Mateo > > > > > > > > Any given ringbuffer is unequivocally tied to one context and one engine. > > > > By setting the appropriate pointers to them, the ringbuffer struct > > > > holds all the infromation you might need to submit a workload for > > > > processing, Execlists style. > > > > > > > > Signed-off-by: Oscar Mateo > > > > --- > > > > drivers/gpu/drm/i915/intel_lrc.c | 2 ++ > > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ > > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +++ > > > > 3 files changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > > > > b/drivers/gpu/drm/i915/intel_lrc.c > > > > index 0a12b8c..2eb7db6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > > @@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct > > intel_context *ctx, > > > > return ret; > > > > } > > > > > > > > + ringbuf->ring = ring; > > > > + ringbuf->ctx = ctx; > > > > ringbuf->size = 32 * PAGE_SIZE; > > > > ringbuf->effective_size = ringbuf->size; > > > > ringbuf->head = 0; > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > index 01e9840..279dda4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > @@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct > > drm_device *dev, > > > > INIT_LIST_HEAD(&ring->active_list); > > > > INIT_LIST_HEAD(&ring->request_list); > > > > ringbuf->size = 32 * PAGE_SIZE; > > > > + ringbuf->ring = ring; > > > > + ringbuf->ctx = ring->default_context; > > > > > > That doesn't make a terribly lot of sense tbh. I fear it's one of > > > these slight confusions which will take tons of patches to clean up. > > > Why exactly do we need the ring->ctx pointer? > > > > > > If we only need this for lrc I want to name it accordingly, to make > > > sure legacy code doesn't grow stupid ideas. And also we should only > > > initialize this in the lrc ctx init then. > > > > > > All patches up to this one merged. > > > > Ok, I've discussed this quickly with Damien on irc. We decided to cut away > > the ring->ctx part of this patch for now to be able to move on. > > -Daniel > As you've seen, removing ringbuffer->ctx causes serious problems with the > plumbing later on. This can be renamed (perhaps to lrc) and removed from > legacy init. > > Each ring buffer belongs to a specific context - it makes sense to me to > keep this information within the ringbuffer structure so that we don't have > to pass the context pointer around everywhere. I agree that it causes trouble with the follow-up patches, but I'm not sold on this being a terrible good idea. After all for ELSP we don't want to submit a ring, we want to submit the full context. So if the code that's supposed to do the execlist ctx submission only has the pointer to the ring object, the layer looks a bit wrong. Same was iirc about the add_request part. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch