Skip to content

Commit fe4751c

Browse files
gfxstranddanvet
authored andcommitted
drm/i915: Drop I915_CONTEXT_PARAM_RINGSIZE
This reverts commit 88be76c ("drm/i915: Allow userspace to specify ringsize on construction"). This API was originally added for OpenCL but the compute-runtime PR has sat open for a year without action so we can still pull it out if we want. I argue we should drop it for three reasons: 1. If the compute-runtime PR has sat open for a year, this clearly isn't that important. 2. It's a very leaky API. Ring size is an implementation detail of the current execlist scheduler and really only makes sense there. It can't apply to the older ring-buffer scheduler on pre-execlist hardware because that's shared across all contexts and it won't apply to the GuC scheduler that's in the pipeline. 3. Having userspace set a ring size in bytes is a bad solution to the problem of having too small a ring. There is no way that userspace has the information to know how to properly set the ring size so it's just going to detect the feature and always set it to the maximum of 512K. This is what the compute-runtime PR does. The scheduler in i915, on the other hand, does have the information to make an informed choice. It could detect if the ring size is a problem and grow it itself. Or, if that's too hard, we could just increase the default size from 16K to 32K or even 64K instead of relying on userspace to do it. Let's drop this API for now and, if someone decides they really care about solving this problem, they can do it properly. Signed-off-by: Jason Ekstrand <[email protected]> Reviewed-by: Daniel Vetter <[email protected]> Signed-off-by: Daniel Vetter <[email protected]> Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
1 parent 520dfc8 commit fe4751c

File tree

5 files changed

+4
-168
lines changed

5 files changed

+4
-168
lines changed

drivers/gpu/drm/i915/Makefile

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ gt-y += \
8989
gt/gen8_ppgtt.o \
9090
gt/intel_breadcrumbs.o \
9191
gt/intel_context.o \
92-
gt/intel_context_param.o \
9392
gt/intel_context_sseu.o \
9493
gt/intel_engine_cs.o \
9594
gt/intel_engine_heartbeat.o \

drivers/gpu/drm/i915/gem/i915_gem_context.c

Lines changed: 2 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1334,63 +1334,6 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
13341334
return err;
13351335
}
13361336

1337-
static int __apply_ringsize(struct intel_context *ce, void *sz)
1338-
{
1339-
return intel_context_set_ring_size(ce, (unsigned long)sz);
1340-
}
1341-
1342-
static int set_ringsize(struct i915_gem_context *ctx,
1343-
struct drm_i915_gem_context_param *args)
1344-
{
1345-
if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
1346-
return -ENODEV;
1347-
1348-
if (args->size)
1349-
return -EINVAL;
1350-
1351-
if (!IS_ALIGNED(args->value, I915_GTT_PAGE_SIZE))
1352-
return -EINVAL;
1353-
1354-
if (args->value < I915_GTT_PAGE_SIZE)
1355-
return -EINVAL;
1356-
1357-
if (args->value > 128 * I915_GTT_PAGE_SIZE)
1358-
return -EINVAL;
1359-
1360-
return context_apply_all(ctx,
1361-
__apply_ringsize,
1362-
__intel_context_ring_size(args->value));
1363-
}
1364-
1365-
static int __get_ringsize(struct intel_context *ce, void *arg)
1366-
{
1367-
long sz;
1368-
1369-
sz = intel_context_get_ring_size(ce);
1370-
GEM_BUG_ON(sz > INT_MAX);
1371-
1372-
return sz; /* stop on first engine */
1373-
}
1374-
1375-
static int get_ringsize(struct i915_gem_context *ctx,
1376-
struct drm_i915_gem_context_param *args)
1377-
{
1378-
int sz;
1379-
1380-
if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
1381-
return -ENODEV;
1382-
1383-
if (args->size)
1384-
return -EINVAL;
1385-
1386-
sz = context_apply_all(ctx, __get_ringsize, NULL);
1387-
if (sz < 0)
1388-
return sz;
1389-
1390-
args->value = sz;
1391-
return 0;
1392-
}
1393-
13941337
int
13951338
i915_gem_user_to_context_sseu(struct intel_gt *gt,
13961339
const struct drm_i915_gem_context_param_sseu *user,
@@ -2036,11 +1979,8 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
20361979
ret = set_persistence(ctx, args);
20371980
break;
20381981

2039-
case I915_CONTEXT_PARAM_RINGSIZE:
2040-
ret = set_ringsize(ctx, args);
2041-
break;
2042-
20431982
case I915_CONTEXT_PARAM_BAN_PERIOD:
1983+
case I915_CONTEXT_PARAM_RINGSIZE:
20441984
default:
20451985
ret = -EINVAL;
20461986
break;
@@ -2068,18 +2008,6 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data)
20682008
return ctx_setparam(arg->fpriv, arg->ctx, &local.param);
20692009
}
20702010

2071-
static int copy_ring_size(struct intel_context *dst,
2072-
struct intel_context *src)
2073-
{
2074-
long sz;
2075-
2076-
sz = intel_context_get_ring_size(src);
2077-
if (sz < 0)
2078-
return sz;
2079-
2080-
return intel_context_set_ring_size(dst, sz);
2081-
}
2082-
20832011
static int clone_engines(struct i915_gem_context *dst,
20842012
struct i915_gem_context *src)
20852013
{
@@ -2124,12 +2052,6 @@ static int clone_engines(struct i915_gem_context *dst,
21242052
}
21252053

21262054
intel_context_set_gem(clone->engines[n], dst);
2127-
2128-
/* Copy across the preferred ringsize */
2129-
if (copy_ring_size(clone->engines[n], e->engines[n])) {
2130-
__free_engines(clone, n + 1);
2131-
goto err_unlock;
2132-
}
21332055
}
21342056
clone->num_engines = n;
21352057
i915_sw_fence_complete(&e->fence);
@@ -2489,11 +2411,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
24892411
args->value = i915_gem_context_is_persistent(ctx);
24902412
break;
24912413

2492-
case I915_CONTEXT_PARAM_RINGSIZE:
2493-
ret = get_ringsize(ctx, args);
2494-
break;
2495-
24962414
case I915_CONTEXT_PARAM_BAN_PERIOD:
2415+
case I915_CONTEXT_PARAM_RINGSIZE:
24972416
default:
24982417
ret = -EINVAL;
24992418
break;

drivers/gpu/drm/i915/gt/intel_context_param.c

Lines changed: 0 additions & 63 deletions
This file was deleted.

drivers/gpu/drm/i915/gt/intel_context_param.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,6 @@
1010

1111
#include "intel_context.h"
1212

13-
int intel_context_set_ring_size(struct intel_context *ce, long sz);
14-
long intel_context_get_ring_size(struct intel_context *ce);
15-
1613
static inline int
1714
intel_context_set_watchdog_us(struct intel_context *ce, u64 timeout_us)
1815
{

include/uapi/drm/i915_drm.h

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1722,24 +1722,8 @@ struct drm_i915_gem_context_param {
17221722
*/
17231723
#define I915_CONTEXT_PARAM_PERSISTENCE 0xb
17241724

1725-
/*
1726-
* I915_CONTEXT_PARAM_RINGSIZE:
1727-
*
1728-
* Sets the size of the CS ringbuffer to use for logical ring contexts. This
1729-
* applies a limit of how many batches can be queued to HW before the caller
1730-
* is blocked due to lack of space for more commands.
1731-
*
1732-
* Only reliably possible to be set prior to first use, i.e. during
1733-
* construction. At any later point, the current execution must be flushed as
1734-
* the ring can only be changed while the context is idle. Note, the ringsize
1735-
* can be specified as a constructor property, see
1736-
* I915_CONTEXT_CREATE_EXT_SETPARAM, but can also be set later if required.
1737-
*
1738-
* Only applies to the current set of engine and lost when those engines
1739-
* are replaced by a new mapping (see I915_CONTEXT_PARAM_ENGINES).
1740-
*
1741-
* Must be between 4 - 512 KiB, in intervals of page size [4 KiB].
1742-
* Default is 16 KiB.
1725+
/* This API has been removed. On the off chance someone somewhere has
1726+
* attempted to use it, never re-use this context param number.
17431727
*/
17441728
#define I915_CONTEXT_PARAM_RINGSIZE 0xc
17451729
/* Must be kept compact -- no holes and well documented */

0 commit comments

Comments
 (0)