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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/01/16 12:27, Chris Wilson wrote:
> On Fri, Jan 15, 2016 at 11:58:32AM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/01/16 11:06, Chris Wilson wrote:
>>> Tvrtko was looking through the execbuffer-ioctl and noticed that the
>>> uABI was tightly coupled to our internal engine identifiers. Close
>>> inspection also revealed that we leak those internal engine identifiers
>>> through the busy-ioctl, and those internal identifiers already do not
>>> match the user identifiers. Fortuitiously, there is only one user of the
>>> set of busy rings from the busy-ioctl, and they only wish to choose
>>> between the RENDER and the BLT engines.
>>>
>>> Let's fix the userspace ABI while we still can.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c         | 18 ++++++++++++++----
>>>   drivers/gpu/drm/i915/intel_lrc.c        |  5 +++++
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c |  5 +++++
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>>>   4 files changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index bb44bad15403..85797813a3de 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>>>   	if (ret)
>>>   		goto unref;
>>>
>>> -	BUILD_BUG_ON(I915_NUM_RINGS > 16);
>>> -	args->busy = obj->active << 16;
>>> -	if (obj->last_write_req)
>>> -		args->busy |= obj->last_write_req->ring->id;
>>> +	args->busy = 0;
>>> +	if (obj->active) {
>>> +		int i;
>>> +
>>> +		for (i = 0; i < I915_NUM_RINGS; i++) {
>>> +			struct drm_i915_gem_request *req;
>>> +
>>> +			req = obj->last_read_req[i];
>>> +			if (req)
>>> +				args->busy |= 1 << (16 + req->ring->exec_id);
>>
>> If I got it right bit 16 was RCS, now will always be clear. And
>> blitter was bit 17 and now is 19.
>
> Sssh. You weren't meant to point that out.
>
> I am willing to take the hit in order to decouple the uABI from
> internals.
>
> I am also willing to codify this busy-ioctl ABI into igt!

Looks like your DDX is the only user not using it in the boolean mode?

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx