| Message ID | 1406217891-8912-9-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 24, 2014 at 05:04:16PM +0100, Thomas Daniel wrote: > From: Oscar Mateo <oscar.mateo@intel.com> > > 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 <oscar.mateo@intel.com> > --- > 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. -Daniel > memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno)); > > init_waitqueue_head(&ring->irq_queue); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 053d004..be40788 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -88,6 +88,9 @@ struct intel_ringbuffer { > struct drm_i915_gem_object *obj; > void __iomem *virtual_start; > > + struct intel_engine_cs *ring; > + struct intel_context *ctx; > + > u32 head; > u32 tail; > int space; > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 <oscar.mateo@intel.com> > > > > 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 <oscar.mateo@intel.com> > > --- > > 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 > -Daniel > > > memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno)); > > > > init_waitqueue_head(&ring->irq_queue); > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index 053d004..be40788 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -88,6 +88,9 @@ struct intel_ringbuffer { > > struct drm_i915_gem_object *obj; > > void __iomem *virtual_start; > > > > + struct intel_engine_cs *ring; > > + struct intel_context *ctx; > > + > > u32 head; > > u32 tail; > > int space; > > -- > > 1.7.9.5 > > > > _______________________________________________ > > 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
> -----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 <oscar.mateo@intel.com> > > > > > > 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 <oscar.mateo@intel.com> > > > --- > > > 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. Thomas. > > > -Daniel > > > > > memset(ring->semaphore.sync_seqno, 0, > > > sizeof(ring->semaphore.sync_seqno)); > > > > > > init_waitqueue_head(&ring->irq_queue); > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > > > b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > index 053d004..be40788 100644 > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > @@ -88,6 +88,9 @@ struct intel_ringbuffer { > > > struct drm_i915_gem_object *obj; > > > void __iomem *virtual_start; > > > > > > + struct intel_engine_cs *ring; > > > + struct intel_context *ctx; > > > + > > > u32 head; > > > u32 tail; > > > int space; > > > -- > > > 1.7.9.5 > > > > > > _______________________________________________ > > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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 <oscar.mateo@intel.com> > > > > > > > > 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 <oscar.mateo@intel.com> > > > > --- > > > > 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
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Wednesday, August 13, 2014 4:16 PM > To: Daniel, Thomas > Cc: Daniel Vetter; 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 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 <oscar.mateo@intel.com> > > > > > > > > > > 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 <oscar.mateo@intel.com> > > > > > --- > > > > > 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. When it comes to the execlist submission (actually as early as the execlist request queueing), the engine and context are indeed used and required. intel_logical_ring_advance_and_submit() is the lrc function analogous to __intel_ring_advance() and I believe the initial creation of intel_lrc.c was actually done by copying intel_ringbuffer.c. This explains why some of the lrc code is perhaps not as it would have been if this had been designed from scratch, and there is room for future improvement. advance_and_submit therefore only gets the ringbuffer struct and uses the context pointer in that struct to get the logical ring context itself. At that point the engine, context and new tail pointer are handed over to the execlist queue backend. Thomas. > > 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
On Thu, Aug 14, 2014 at 03:09:45PM +0000, Daniel, Thomas wrote: > When it comes to the execlist submission (actually as early as the execlist > request queueing), the engine and context are indeed used and required. > intel_logical_ring_advance_and_submit() is the lrc function analogous to > __intel_ring_advance() and I believe the initial creation of intel_lrc.c > was actually done by copying intel_ringbuffer.c. This explains why some of > the lrc code is perhaps not as it would have been if this had been designed > from scratch, and there is room for future improvement. > advance_and_submit therefore only gets the ringbuffer struct and uses > the context pointer in that struct to get the logical ring context itself. At > that point the engine, context and new tail pointer are handed over to the > execlist queue backend. I guess I need to clarify: Does it make sense to move the ELSP respectively the submission to the execlist scheduler queue out of there up a few levels into the execlist cmd submission function? Is it possible or is there some technical reason that I'm overlooking? I want to know what exactly I'm dealing with here before I sign up for it by merging the patches as-is and asking for a cleanup. I doesn't look bad really, but there's always a good chance that I've overlooked a bigger dragon. Since you have the patches and worked with them I'm asking you such explorative questions. Ofc I can do this checking myself, but that takes time ... This doesn't mean that you have to implement the changes, just be reasonable confident that it will work out as a cleanup on top. -Daniel
On Thu, Aug 14, 2014 at 05:32:28PM +0200, Daniel Vetter wrote: > On Thu, Aug 14, 2014 at 03:09:45PM +0000, Daniel, Thomas wrote: > > When it comes to the execlist submission (actually as early as the execlist > > request queueing), the engine and context are indeed used and required. > > intel_logical_ring_advance_and_submit() is the lrc function analogous to > > __intel_ring_advance() and I believe the initial creation of intel_lrc.c > > was actually done by copying intel_ringbuffer.c. This explains why some of > > the lrc code is perhaps not as it would have been if this had been designed > > from scratch, and there is room for future improvement. > > advance_and_submit therefore only gets the ringbuffer struct and uses > > the context pointer in that struct to get the logical ring context itself. At > > that point the engine, context and new tail pointer are handed over to the > > execlist queue backend. > > I guess I need to clarify: Does it make sense to move the ELSP > respectively the submission to the execlist scheduler queue out of there > up a few levels into the execlist cmd submission function? Is it possible > or is there some technical reason that I'm overlooking? > > I want to know what exactly I'm dealing with here before I sign up for it > by merging the patches as-is and asking for a cleanup. I doesn't look bad > really, but there's always a good chance that I've overlooked a bigger > dragon. > > Since you have the patches and worked with them I'm asking you such > explorative questions. Ofc I can do this checking myself, but that takes > time ... This doesn't mean that you have to implement the changes, just be > reasonable confident that it will work out as a cleanup on top. To clarify more the context: Currently you're replies sound like "This is what it looks like and I don't really know why nor whether we can change that". That's not confidence instilling and that makes maintainers reluctant to merge patches for fear of needing to fix things themselves ;-) -Daniel
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Thursday, August 14, 2014 4:37 PM > To: Daniel, Thomas > Cc: Daniel Vetter; 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 Thu, Aug 14, 2014 at 05:32:28PM +0200, Daniel Vetter wrote: > > On Thu, Aug 14, 2014 at 03:09:45PM +0000, Daniel, Thomas wrote: > > > When it comes to the execlist submission (actually as early as the > > > execlist request queueing), the engine and context are indeed used and > required. > > > intel_logical_ring_advance_and_submit() is the lrc function > > > analogous to > > > __intel_ring_advance() and I believe the initial creation of > > > intel_lrc.c was actually done by copying intel_ringbuffer.c. This > > > explains why some of the lrc code is perhaps not as it would have > > > been if this had been designed from scratch, and there is room for future > improvement. > > > advance_and_submit therefore only gets the ringbuffer struct and > > > uses the context pointer in that struct to get the logical ring > > > context itself. At that point the engine, context and new tail > > > pointer are handed over to the execlist queue backend. > > > > I guess I need to clarify: Does it make sense to move the ELSP > > respectively the submission to the execlist scheduler queue out of > > there up a few levels into the execlist cmd submission function? Is it > > possible or is there some technical reason that I'm overlooking? Yes this would make sense, and we already have a separate emit_request vfunc which is only used in lrc mode so we can for example change the signature to accept a drm_i915_gem_request* directly and take the ctx and engine from there. > > > > I want to know what exactly I'm dealing with here before I sign up for > > it by merging the patches as-is and asking for a cleanup. I doesn't > > look bad really, but there's always a good chance that I've overlooked > > a bigger dragon. > > > > Since you have the patches and worked with them I'm asking you such > > explorative questions. Ofc I can do this checking myself, but that > > takes time ... This doesn't mean that you have to implement the > > changes, just be reasonable confident that it will work out as a cleanup on > top. Understood. > > To clarify more the context: Currently you're replies sound like "This is what it > looks like and I don't really know why nor whether we can change that". You're right, I don't know why it was done this way. But I can see that there is no problem to change it later - it's only a piece of code after all... > That's not confidence instilling and that makes maintainers reluctant to > merge patches for fear of needing to fix things themselves > ;-) If it helps, I can tell you that several guys in our team are working with this code and we have a vested interest in making sure the quality is as high as possible. Cheers, Thomas. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Aug 14, 2014 at 03:56:20PM +0000, Daniel, Thomas wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Thursday, August 14, 2014 4:37 PM > > To: Daniel, Thomas > > Cc: Daniel Vetter; 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 Thu, Aug 14, 2014 at 05:32:28PM +0200, Daniel Vetter wrote: > > > On Thu, Aug 14, 2014 at 03:09:45PM +0000, Daniel, Thomas wrote: > > > > When it comes to the execlist submission (actually as early as the > > > > execlist request queueing), the engine and context are indeed used and > > required. > > > > intel_logical_ring_advance_and_submit() is the lrc function > > > > analogous to > > > > __intel_ring_advance() and I believe the initial creation of > > > > intel_lrc.c was actually done by copying intel_ringbuffer.c. This > > > > explains why some of the lrc code is perhaps not as it would have > > > > been if this had been designed from scratch, and there is room for future > > improvement. > > > > advance_and_submit therefore only gets the ringbuffer struct and > > > > uses the context pointer in that struct to get the logical ring > > > > context itself. At that point the engine, context and new tail > > > > pointer are handed over to the execlist queue backend. > > > > > > I guess I need to clarify: Does it make sense to move the ELSP > > > respectively the submission to the execlist scheduler queue out of > > > there up a few levels into the execlist cmd submission function? Is it > > > possible or is there some technical reason that I'm overlooking? > Yes this would make sense, and we already have a separate emit_request > vfunc which is only used in lrc mode so we can for example change the > signature to accept a drm_i915_gem_request* directly and take the ctx > and engine from there. Hm, I didn't spot the emit_request vfunc yet. Probably another one that I'll ask you to fold in ;-) > > > I want to know what exactly I'm dealing with here before I sign up for > > > it by merging the patches as-is and asking for a cleanup. I doesn't > > > look bad really, but there's always a good chance that I've overlooked > > > a bigger dragon. > > > > > > Since you have the patches and worked with them I'm asking you such > > > explorative questions. Ofc I can do this checking myself, but that > > > takes time ... This doesn't mean that you have to implement the > > > changes, just be reasonable confident that it will work out as a cleanup on > > top. > Understood. > > > > > To clarify more the context: Currently you're replies sound like "This is what it > > looks like and I don't really know why nor whether we can change that". > You're right, I don't know why it was done this way. But I can see that there > is no problem to change it later - it's only a piece of code after all... > > > That's not confidence instilling and that makes maintainers reluctant to > > merge patches for fear of needing to fix things themselves > > ;-) > If it helps, I can tell you that several guys in our team are working with this > code and we have a vested interest in making sure the quality is as high as > possible. Ok, I think you get the hang of how this "guide your maintainer into accepting stuff" game works ;-) I'll take your word for it that there's no dragon hiding and that you'll bravely fight it anyway and will merge in a few more patches. And I'll do a JIRA to jot down the restructuring we need to do. Merging might be a bit slower since I'm heading to Chicago on Saturday already, so I might ask you to resend the remaining patches. Cheers, Daniel
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; memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno)); init_waitqueue_head(&ring->irq_queue); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 053d004..be40788 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -88,6 +88,9 @@ struct intel_ringbuffer { struct drm_i915_gem_object *obj; void __iomem *virtual_start; + struct intel_engine_cs *ring; + struct intel_context *ctx; + u32 head; u32 tail; int space;