Re: [PATCH 04/51] drm/i915: Merged the many do_execbuf() parameters into a structure

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 27, 2015 at 06:22:43PM +0000, John Harrison wrote:
> On 27/02/2015 13:35, Daniel Vetter wrote:
> >On Fri, Feb 27, 2015 at 12:22:42PM +0000, John Harrison wrote:
> >>On 25/02/2015 21:52, Daniel Vetter wrote:
> >>>On Fri, Feb 13, 2015 at 11:48:13AM +0000, John.C.Harrison@Intel.com wrote:
> >>>>From: John Harrison <John.C.Harrison@Intel.com>
> >>>>
> >>>>The do_execbuf() function takes quite a few parameters. The actual set of
> >>>>parameters is going to change with the conversion to passing requests around.
> >>>>Further, it is due to grow massively with the arrival of the GPU scheduler.
> >>>>
> >>>>This patch simplies the prototype by passing a parameter structure instead.
> >>>>Changing the parameter set in the future is then simply a matter of
> >>>>adding/removing items to the structure.
> >>>>
> >>>>Note that the structure does not contain absolutely everything that is passed
> >>>>in. This is because the intention is to use this structure more extensively
> >>>>later in this patch series and more especially in the GPU scheduler that is
> >>>>coming soon. The latter requires hanging on to the structure as the final
> >>>>hardware submission can be delayed until long after the execbuf IOCTL has
> >>>>returned to user land. Thus it is unsafe to put anything in the structure that
> >>>>is local to the IOCTL call itself - such as the 'args' parameter. All entries
> >>>>must be copies of data or pointers to structures that are reference counted in
> >>>>someway and guaranteed to exist for the duration of the batch buffer's life.
> >>>>
> >>>>For: VIZ-5115
> >>>>Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> >>>>---
> >>>>  drivers/gpu/drm/i915/i915_drv.h            |   27 +++++++-------
> >>>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   56 ++++++++++++++++++----------
> >>>>  drivers/gpu/drm/i915/intel_lrc.c           |   26 +++++++------
> >>>>  drivers/gpu/drm/i915/intel_lrc.h           |    9 ++---
> >>>>  4 files changed, 67 insertions(+), 51 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>index e90b786..e6d616b 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>@@ -1640,6 +1640,16 @@ struct i915_workarounds {
> >>>>  	u32 count;
> >>>>  };
> >>>>+struct i915_execbuffer_params {
> >>>>+	struct drm_device               *dev;
> >>>>+	struct drm_file                 *file;
> >>>>+	uint32_t                        dispatch_flags;
> >>>>+	uint32_t                        batch_obj_vm_offset;
> >>>>+	struct intel_engine_cs          *ring;
> >>>>+	struct drm_i915_gem_object      *batch_obj;
> >>>>+	struct intel_context            *ctx;
> >>>>+};
> >>>tbh I'm not a fan of parameter objects in C. C is verbose and explicit.
> >>>
> >>>If we add the request then we can remove ring and ctx, which already
> >>>improves things. We also have the eb structure we use to pass around a
> >>>pile of things, which we could somewhat reuse here. What else do you plan
> >>>to add? Just want to figure out whether this is really required ...
> >>>-Daniel
> >>The major advantage of this is when the GPU scheduler arrives. It splits the
> >>execbuffer code path in half - front half is all the software state
> >>verification and management, back half is the actual hardware writes to post
> >>the buffer. This structure is used to bridge the two. It contains all the
> >>state created by the front half that is later used by the back half. It is
> >>saved away with the request/scheduler node until such a time as the back
> >>half is called. Hence some kind of structure is required and it does not
> >>really make sense to add all this information to the generic request
> >>structure. Also, with the full pre-emptive scheduler, the structure grows to
> >>about twenty entries and passing that quantity of individual parameters to a
> >>function is just unwieldly!
> >Well we already have an execbuf tracking structure with eb_vmas. And then
> >there's requests. So if this is more than just a parameter object I wonder
> >whether we shouldn't reuse either of these. Adding them all to requests is
> >imo totally ok.
> >
> >Oh and a bikeshed aside: imo the ->do_exebuf name is really confusing,
> >since it clashes with the the various do_execbuf functions we have which
> >are at a higher level. Also the implementations all have a _submission
> >postfix. Imo better names would be do_submission or engine_submit or
> >something simlar. The only caveat is that we need to make sure the
> >lifetime rules for any pointers are ok. E.g. before we return from the
> >ioctl we need to clear out drm_i915_gem_execbuffer2 *args pointers if we
> >store them in the request structure.
> 
> The lifetime issues is part of the reason for having a dedicated execbuf
> params structure. Some of the contents of the args block are copied out and
> saved away for future reference and they are only relevant to the execbuf
> code. IMHO, keeping all of these things in one place is much more neat and
> tidy (and thus easier to read and easier to maintain) than overloading other
> structures with large amounts of stuff that isn't relevant. The eb_vma
> structure is freed at the end of the front half and not kept around for that
> back half. Requests are used for stuff that isn't an execbuffer call.

Hm I still think just putting all that stuff into requests makes more
sense. Requests' primary reason for existence is to track execbufs after
all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx