On Thu, Jun 18, 2015 at 04:24:53PM +0200, Daniel Vetter wrote: > On Thu, Jun 18, 2015 at 01:59:13PM +0100, John Harrison wrote: > > On 18/06/2015 13:21, Chris Wilson wrote: > > >On Thu, Jun 18, 2015 at 01:14:56PM +0100, John.C.Harrison@xxxxxxxxx wrote: > > >>From: John Harrison <John.C.Harrison@xxxxxxxxx> > > >> > > >>The plan is to pass requests around as the basic submission tracking structure > > >>rather than rings and contexts. This patch updates the i915_gem_object_sync() > > >>code path. > > >> > > >>v2: Much more complex patch to share a single request between the sync and the > > >>page flip. The _sync() function now supports lazy allocation of the request > > >>structure. That is, if one is passed in then that will be used. If one is not, > > >>then a request will be allocated and passed back out. Note that the _sync() code > > >>does not necessarily require a request. Thus one will only be created until > > >>certain situations. The reason the lazy allocation must be done within the > > >>_sync() code itself is because the decision to need one or not is not really > > >>something that code above can second guess (except in the case where one is > > >>definitely not required because no ring is passed in). > > >> > > >>The call chains above _sync() now support passing a request through which most > > >>callers passing in NULL and assuming that no request will be required (because > > >>they also pass in NULL for the ring and therefore can't be generating any ring > > >>code). > > >> > > >>The exeception is intel_crtc_page_flip() which now supports having a request > > >>returned from _sync(). If one is, then that request is shared by the page flip > > >>(if the page flip is of a type to need a request). If _sync() does not generate > > >>a request but the page flip does need one, then the page flip path will create > > >>its own request. > > >> > > >>v3: Updated comment description to be clearer about 'to_req' parameter (Tomas > > >>Elf review request). Rebased onto newer tree that significantly changed the > > >>synchronisation code. > > >> > > >>v4: Updated comments from review feedback (Tomas Elf) > > >> > > >>For: VIZ-5115 > > >>Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx> > > >>Reviewed-by: Tomas Elf <tomas.elf@xxxxxxxxx> > > >>--- > > >> drivers/gpu/drm/i915/i915_drv.h | 4 ++- > > >> drivers/gpu/drm/i915/i915_gem.c | 48 +++++++++++++++++++++------- > > >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > > >> drivers/gpu/drm/i915/intel_display.c | 17 +++++++--- > > >> drivers/gpu/drm/i915/intel_drv.h | 3 +- > > >> drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > > >> drivers/gpu/drm/i915/intel_lrc.c | 2 +- > > >> drivers/gpu/drm/i915/intel_overlay.c | 2 +- > > >> 8 files changed, 57 insertions(+), 23 deletions(-) > > >> > > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > >>index 64a10fa..f69e9cb 100644 > > >>--- a/drivers/gpu/drm/i915/i915_drv.h > > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > > >>@@ -2778,7 +2778,8 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) > > >> int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); > > >> int i915_gem_object_sync(struct drm_i915_gem_object *obj, > > >>- struct intel_engine_cs *to); > > >>+ struct intel_engine_cs *to, > > >>+ struct drm_i915_gem_request **to_req); > > >Nope. Did you forget to reorder the code to ensure that the request is > > >allocated along with the context switch at the start of execbuf? > > >-Chris > > > > > Not sure what you are objecting to? If you mean the lazily allocated request > > then that is for page flip code not execbuff code. If we get here from an > > execbuff call then the request will definitely have been allocated and will > > be passed in. Whereas the page flip code may or may not require a request > > (depending on whether MMIO or ring flips are in use. Likewise the sync code > > may or may not require a request (depending on whether there is anything to > > sync to or not). There is no point allocating and submitting an empty > > request in the MMIO/idle case. Hence the sync code needs to be able to use > > an existing request or create one if none already exists. > > I guess Chris' comment was that if you have a non-NULL to, then you better > have a non-NULL to_req. And since we link up reqeusts to the engine > they'll run on the former shouldn't be required any more. So either that's > true and we can remove the to or we don't understand something yet (and > perhaps that should be done as a follow-up). I am sure I sent a patch that outlined in great detail how that we need only the request parameter in i915_gem_object_sync(), for handling both execbuffer, pipelined pin_and_fence and synchronous pin_and_fence. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
