[Intel-gfx] [PATCH] drm/i915: Decouple execbuf uAPI from internal implementation
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Apr 5 09:17:25 UTC 2016
Hi,
On 25/03/16 13:18, Kukanova, Svetlana wrote:
> Hi everyone,
>
> Yes, this breaks userspace ABI and in particular it broke VTune work.
> Ring ids are seen via i915 tracepoints, and VTune Amplifier uses them.
> We were relying on the old ring ids, and assuming that the new rings would be added to the end of the enum.
>
> I'm objecting just now because now this driver change reached our internal users and they complained that VTune is reporting DMA packets on wrong engines.
>
> I would request this change (the enum intel_ring_id change) to be rolled back. Hope, it's still possible.
This one will be the one for Daniel. Personally I didn't think
tracepoints count as ABI, if you consider they can start and stop making
sense just by the virtue of internal driver refactoring.
But as a quick Google search shows, there is some controversy on the
topic, even if it is still questionable for device drivers.
Also, could you not detect the i915 version somehow from VTune and adapt?
Regards,
Tvrtko
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Chris Wilson
> Sent: Friday, January 15, 2016 1:09 AM
> To: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>; Intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Decouple execbuf uAPI from internal implementation
>
> On Thu, Jan 14, 2016 at 03:02:59PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> At the moment execbuf ring selection is fully coupled to internal ring
>> ids which is not a good thing on its own.
>>
>> This dependency is also spread between two source files and not
>> spelled out at either side which makes it hidden and fragile.
>>
>> This patch decouples this dependency by introducing an explicit
>> translation table of execbuf uAPI to ring id close to the only call
>> site (i915_gem_do_execbuffer).
>>
>> This way we are free to change driver internal implementation details
>> without breaking userspace. All state relating to the uAPI is now
>> contained in, or next to, i915_gem_do_execbuffer.
>
> I was hesistant at first, since "why?" isn't fully developed, but the code won me over.
>
>> +#define I915_USER_RINGS (4)
>> +
>> +static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = {
>> + [I915_EXEC_DEFAULT] = RCS,
>> + [I915_EXEC_RENDER] = RCS,
>> + [I915_EXEC_BLT] = BCS,
>> + [I915_EXEC_BSD] = VCS,
>> + [I915_EXEC_VEBOX] = VECS
>> +};
>
> I was wondering whether packing here mattered at all, but we're under a cacheline so highly unlikely.
>
>> static int
>> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>> struct drm_file *file,
>> @@ -1393,6 +1393,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>> struct i915_execbuffer_params params_master; /* XXX: will be removed later */
>> struct i915_execbuffer_params *params = ¶ms_master;
>> const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
>> + unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>> u32 dispatch_flags;
>> int ret;
>> bool need_relocs;
>> @@ -1414,49 +1415,39 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>> if (args->flags & I915_EXEC_IS_PINNED)
>> dispatch_flags |= I915_DISPATCH_PINNED;
>>
>> - if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
>> - DRM_DEBUG("execbuf with unknown ring: %d\n",
>> - (int)(args->flags & I915_EXEC_RING_MASK));
>> + if (user_ring_id > I915_USER_RINGS) {
>> + DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
>> return -EINVAL;
>> }
>>
>> - if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) &&
>> + if ((user_ring_id != I915_EXEC_BSD) &&
>> ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
>> DRM_DEBUG("execbuf with non bsd ring but with invalid "
>> "bsd dispatch flags: %d\n", (int)(args->flags));
>> return -EINVAL;
>> - }
>> -
>> - if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
>> - ring = &dev_priv->ring[RCS];
>> - else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
>> - if (HAS_BSD2(dev)) {
>> - int ring_id;
>> -
>> - switch (args->flags & I915_EXEC_BSD_MASK) {
>> - case I915_EXEC_BSD_DEFAULT:
>> - ring_id = gen8_dispatch_bsd_ring(dev, file);
>> - ring = &dev_priv->ring[ring_id];
>> - break;
>> - case I915_EXEC_BSD_RING1:
>> - ring = &dev_priv->ring[VCS];
>> - break;
>> - case I915_EXEC_BSD_RING2:
>> - ring = &dev_priv->ring[VCS2];
>> - break;
>> - default:
>> - DRM_DEBUG("execbuf with unknown bsd ring: %d\n",
>> - (int)(args->flags & I915_EXEC_BSD_MASK));
>> - return -EINVAL;
>> - }
>> - } else
>> - ring = &dev_priv->ring[VCS];
>> - } else
>> - ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1];
>> + }
>> +
>> + if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev)) {
>
> HAS_BSD2(dev_priv)
>
>> + unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>> +
>> + if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
>> + bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
>> + } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
>> + bsd_idx <= I915_EXEC_BSD_RING2) {
>> + bsd_idx--;
>> + } else {
>> + DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
>> + bsd_idx);
>> + return -EINVAL;
>> + }
>> +
>> + ring = &dev_priv->ring[_VCS(bsd_idx)];
>> + } else {
>> + ring = &dev_priv->ring[user_ring_map[user_ring_id]];
>> + }
>>
>> if (!intel_ring_initialized(ring)) {
>> - DRM_DEBUG("execbuf with invalid ring: %d\n",
>> - (int)(args->flags & I915_EXEC_RING_MASK));
>> + DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
>> return -EINVAL;
>> }
>
> One question, how does this look if we move this section to its own
> function:
>
> ring = eb_select_ring(dev_priv, file, args); if (IS_ERR(ring))
> return PTR_ERR(ring);
>
> Do you keep your code size reduction?
>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 7349d9258191..fdc220fc2b18 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -148,14 +148,14 @@ struct i915_ctx_workarounds { struct
>> intel_engine_cs {
>> const char *name;
>> enum intel_ring_id {
>> - RCS = 0x0,
>> - VCS,
>> + RCS = 0,
>> BCS,
>> - VECS,
>> - VCS2
>> + VCS,
>> + VCS2, /* Keep instances of the same type engine together. */
>> + VECS
>
> Technically, this breaks userspace ABI elsewhere. :| Though the only user of the id mask is only looking for RCS==0 vs the reset.
>
> So I think we would cope, but to be extra safe we could just avoid reshuffling the ids. Let me sleep on the implications. We may say just break that ABI whilst we still can and do a reverse-map to EXEC bit.
> Hmm.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list