Re: [PATCH 1/2] drm/vc4: Add a mechanism to easily extend CL submissions

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Fri, 22 Dec 2017 12:53:36 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > The number of attributes/objects you can pass to the
> > DRM_IOCTL_VC4_SUBMIT_CL ioctl is limited by the drm_vc4_submit_cl struct
> > size.
> >
> > Add a mechanism to easily pass extra attributes when submitting a CL to
> > the V3D engine.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---  
> 
> > diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
> > index 52263b575bdc..ddcaa72da82a 100644
> > --- a/include/uapi/drm/vc4_drm.h
> > +++ b/include/uapi/drm/vc4_drm.h
> > @@ -70,6 +70,50 @@ struct drm_vc4_submit_rcl_surface {
> >  };
> >  
> >  /**
> > + * @VC4_BIN_CL_CHUNK: binner CL chunk
> > + */
> > +enum {
> > +	VC4_BIN_CL_CHUNK,
> > +};
> > +
> > +/**
> > + * struct drm_vc4_submit_cl_chunk - dummy chunk
> > + * @type: extension type
> > + * @pad: unused, should be set to zero
> > + *
> > + * Meant to be used for chunks that do not require extra parameters.
> > + */
> > +struct drm_vc4_submit_cl_dummy_chunk {
> > +	__u32 type;
> > +	__u32 pad[3];
> > +};
> > +
> > +/**
> > + * struct drm_vc4_submit_cl_bin_chunk - binner CL chunk
> > + *
> > + * @type: extention type, should be set to %VC4_BIN_CL_CHUNK
> > + * @size: size in bytes of the binner CL
> > + * @ptr: userspace pointer to the binner CL
> > + */
> > +struct drm_vc4_submit_cl_bin_chunk {
> > +	__u32 type;
> > +	__u32 size;
> > +	__u64 ptr;
> > +};
> > +
> > +/**
> > + * union drm_vc4_submit_cl_chunk - CL chunk
> > + *
> > + * CL chunks allow us to easily extend the set of arguments one can pass
> > + * to the submit CL ioctl without having to add new ioctls/struct everytime
> > + * we run out of free fields in the drm_vc4_submit_cl struct.
> > + */
> > +union drm_vc4_submit_cl_chunk {
> > +	struct drm_vc4_submit_cl_dummy_chunk dummy;
> > +	struct drm_vc4_submit_cl_bin_chunk bin;
> > +};
> > +
> > +/**
> >   * struct drm_vc4_submit_cl - ioctl argument for submitting commands to the 3D
> >   * engine.
> >   *
> > @@ -83,14 +127,23 @@ struct drm_vc4_submit_rcl_surface {
> >   * BO.
> >   */
> >  struct drm_vc4_submit_cl {
> > -	/* Pointer to the binner command list.
> > -	 *
> > -	 * This is the first set of commands executed, which runs the
> > -	 * coordinate shader to determine where primitives land on the screen,
> > -	 * then writes out the state updates and draw calls necessary per tile
> > -	 * to the tile allocation BO.
> > -	 */
> > -	__u64 bin_cl;
> > +	union {
> > +		/* Pointer to the binner command list.
> > +		 *
> > +		 * This is the first set of commands executed, which runs the
> > +		 * coordinate shader to determine where primitives land on
> > +		 * the screen, then writes out the state updates and draw calls
> > +		 * necessary per tile to the tile allocation BO.
> > +		 */
> > +		__u64 bin_cl;
> > +
> > +		/* Pointer to an array of CL chunks.
> > +		 *
> > +		 * This is now the preferred way of passing optional attributes
> > +		 * when submitting a job.
> > +		 */
> > +		__u64 cl_chunks;
> > +	};
> >  
> >  	/* Pointer to the shader records.
> >  	 *
> > @@ -120,8 +173,14 @@ struct drm_vc4_submit_cl {
> >  	__u64 uniforms;
> >  	__u64 bo_handles;
> >  
> > -	/* Size in bytes of the binner command list. */
> > -	__u32 bin_cl_size;
> > +	union {
> > +		/* Size in bytes of the binner command list. */
> > +		__u32 bin_cl_size;
> > +
> > +		/* Number of entries in the CL extension array. */
> > +		__u32 num_cl_chunks;
> > +	};
> > +
> >  	/* Size in bytes of the set of shader records. */
> >  	__u32 shader_rec_size;
> >  	/* Number of shader records.
> > @@ -167,6 +226,7 @@ struct drm_vc4_submit_cl {
> >  #define VC4_SUBMIT_CL_FIXED_RCL_ORDER			(1 << 1)
> >  #define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X		(1 << 2)
> >  #define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y		(1 << 3)
> > +#define VC4_SUBMIT_CL_EXTENDED				(1 << 4)
> >  	__u32 flags;
> >  
> >  	/* Returned value of the seqno of this render job (for the
> > @@ -308,6 +368,7 @@ struct drm_vc4_get_hang_state {
> >  #define DRM_VC4_PARAM_SUPPORTS_THREADED_FS	5
> >  #define DRM_VC4_PARAM_SUPPORTS_FIXED_RCL_ORDER	6
> >  #define DRM_VC4_PARAM_SUPPORTS_MADVISE		7
> > +#define DRM_VC4_PARAM_SUPPORTS_EXTENDED_CL	8
> >  
> >  struct drm_vc4_get_param {
> >  	__u32 param;
> > -- 
> > 2.11.0  
> 
> The vivante folks just extended their batch submit for performance
> monitors, and I was surprised to see that they actually extended their
> struct (without even a flag indicating that userspace was submitting an
> extended struct), which I thought we couldn't do.  Apparently 6 years
> ago(!) the DRM core support was changed so that the driver always gets
> an ioctl arg of the size it requested, and if userspace submitted
> shorter then only the shorter amount is copied in/out and the rest is
> zero-padded.
> 
> That means we could avoid this union stuff and even the whole idea of
> chunks, and just have a single extra id for the perfmon to use in this
> exec.  (assuming that 0 isn't a valid perfmon handle).

0 was a valid ID in my implementation, but I can easily exclude it.

> 
> Does this sound good to you?  It seems like it would be a lot cleaner.

Yep, I'll rework the patch to just extend the drm_vc4_submit_cl
struct (add a new u32 perfmonid field).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel