Re: [PATCH] drm/vc4: Add the DRM_IOCTL_VC4_GEM_MADVISE ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 27, 2017 at 11:49:21AM -0700, Eric Anholt wrote:
> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > This ioctl will allow us to purge inactive userspace buffers when the
> > system is running out of contiguous memory.
> >
> > For now, the purge logic is rather dumb in that it does not try to
> > release only the amount of BO needed to meet the last CMA alloc request
> > but instead purges all objects placed in the purgeable pool as soon as
> > we experience a CMA allocation failure.
> >
> > Note that the in-kernel BO cache is always purged before the purgeable
> > cache because those objects are known to be unused while objects marked
> > as purgeable by a userspace application/library might have to be
> > restored when they are marked back as unpurgeable, which can be
> > expensive.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > Hello,
> >
> > Updates to libdrm, mesa and igt making use of this kernel feature can
> > be found on my github repos [1][2][3].
> >
> > There's currently no debugfs hook to manually force a purge, but this
> > is being discussed and will probably be added in v2.
> >
> > Regards,
> 
> > diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
> > index afae87004963..c01b93d453db 100644
> > --- a/include/uapi/drm/vc4_drm.h
> > +++ b/include/uapi/drm/vc4_drm.h
> > @@ -41,6 +41,7 @@ extern "C" {
> >  #define DRM_VC4_SET_TILING                        0x08
> >  #define DRM_VC4_GET_TILING                        0x09
> >  #define DRM_VC4_LABEL_BO                          0x0a
> > +#define DRM_VC4_GEM_MADVISE                       0x0b
> >  
> >  #define DRM_IOCTL_VC4_SUBMIT_CL           DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_SUBMIT_CL, struct drm_vc4_submit_cl)
> >  #define DRM_IOCTL_VC4_WAIT_SEQNO          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_WAIT_SEQNO, struct drm_vc4_wait_seqno)
> > @@ -53,6 +54,7 @@ extern "C" {
> >  #define DRM_IOCTL_VC4_SET_TILING          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_SET_TILING, struct drm_vc4_set_tiling)
> >  #define DRM_IOCTL_VC4_GET_TILING          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_GET_TILING, struct drm_vc4_get_tiling)
> >  #define DRM_IOCTL_VC4_LABEL_BO            DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_LABEL_BO, struct drm_vc4_label_bo)
> > +#define DRM_IOCTL_VC4_GEM_MADVISE         DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_GEM_MADVISE, struct drm_vc4_gem_madvise)
> >  
> >  struct drm_vc4_submit_rcl_surface {
> >  	__u32 hindex; /* Handle index, or ~0 if not present. */
> > @@ -333,6 +335,16 @@ struct drm_vc4_label_bo {
> >  	__u64 name;
> >  };
> >  
> > +#define VC4_MADV_WILLNEED			0
> > +#define VC4_MADV_DONTNEED			1
> > +#define __VC4_MADV_PURGED			2
> > +
> > +struct drm_vc4_gem_madvise {
> > +	__u32 handle;
> > +	__u32 madv;
> > +	__u32 retained;
> > +};
> 
> danvet had a note in
> http://blog.ffwll.ch/2013/11/botching-up-ioctls.html:
> 
>     Pad the entire struct to a multiple of 64bits - the structure size
>     will otherwise differ on 32bit versus 64bit. Which hurts when
>     passing arrays of structures to the kernel. Or with the ioctl
>     structure size checking that e.g. the drm core does.
> 
> I'm surprised that i915's ioctl didn't do this or have compat code to
> handle it.

This advise is defensive just in case you ever make an array of any of
your uabi structs, and there's a 64 bit value in there somewhere. It only
matters for that case really. But since gpus have a few of those ioctls
(especially command submission) I figured better safe than sorry.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel