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

[Tvrtko Ursulin]

On 15/01/16 16:19, Chris Wilson wrote:
> 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.)
>           */
>

Very good, r-b on the resulting patch.

Regards,

Tvrtko