[Intel-gfx] [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

On Fri, Jan 15, 2016 at 04:02:52PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/01/16 13:57, Chris Wilson wrote:
> >On Fri, Jan 15, 2016 at 01:22:39PM +0000, Tvrtko Ursulin wrote:
> >>Looks like your DDX is the only user not using it in the boolean mode?
> >
> >As far as I am aware, that is the only user that worries about which
> >engine the object is currently active on.
> >
> >>And libdrm is a bit confused in its return statements:
> >>
> >>         ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> >>         if (ret == 0) {
> >>                 bo_gem->idle = !busy.busy;
> >>                 return busy.busy;
> >>         } else {
> >>                 return false;
> >>         }
> >>         return (ret == 0 && busy.busy);
> >>
> >>Looks like it was a boolean as well until commit
> >>02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident
> >>started exposing the bits.
> >
> >Hmm, libdrm bo_is_busy() was always meant to be boolean and that patch
> >postdates when we started storing read/write bits in the return value.
> >So definitely an unintentional leakage.
> 
> In that case I think just respin with comment corrections in uapi
> header for drm_i915_gem_busy?

	/** Return busy status
         *
         * A return of 0 implies that the object is idle (after
         * having flushed any pending activity), and a non-zero return that
         * the object is still in-flight on the GPU. (The GPU has not yet
         * signaled completion for all pending requests that reference the
         * object.)
         *
         * The returned dword is split into two fields to indicate both
         * the engines on which the object is being read, and the
         * engine on which is currently being writtern to (if any).
         *
         * The low word (bits 0:15) indicate if the object is being written
         * to by any engine (there can only be one, as the GEM implicit
         * synchronisation rules force writes to be serialised.) Only the
         * engine for last write is reported.
         *
         * The high word (bits 16:31) are a bitmask of which engines are
         * currently reading from the object.
         *
         * The value of each engine is the same as specified in the
         * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc.
         * (Note I915_EXEC_DEFAULT is a symbolic value and is mapped to
         * the I915_EXEC_RENDER engine for execution, and so never reported
         * as active itself.)
         */

-- 
Chris Wilson, Intel Open Source Technology Centre