On Thu, Oct 24, 2013 at 06:24:18PM -0200, Rodrigo Vivi wrote:
> If Userspace isn't using MI_PREDICATE all slices must be enabled for
> backward compatibility.
>
> If I915_EXEC_USE_PREDICATE isn't set and defaul is set to half, kernel will force
> all slices on.
>
> v2: fix the inverted logic for backwards compatibility
> USE_PREDICATE unset force gt_full when defaul is half
> instead of GT_FULL flag.
>
> v3: Accepting Chris's suggestions: better variable names;
> better logic around state_default x legacy_userspace_busy;
> remove unecessary mutex;
>
> v4: Accepting more suggestions from Chris:
> * Send all LRIs in only one block and don't ignore if it fails.
> * function name and cleaner code on forcing_full.
>
> CC: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> CC: Eric Anholt <eric@xxxxxxxxxx>
> CC: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8 ++++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 63 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 11 ++++++
> drivers/gpu/drm/i915/i915_sysfs.c | 3 ++
> drivers/gpu/drm/i915/intel_display.c | 5 +++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 45 ++++++++++++++++++++-
> include/uapi/drm/i915_drm.h | 8 +++-
> 8 files changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 685fb1d..67bbbce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1219,6 +1219,12 @@ struct i915_package_c8 {
> } regsave;
> };
>
> +struct i915_gt_slices {
> + int state_default;
> + int legacy_userspace_busy;
> + struct mutex lock; /* locks access to this scruct and slice registers */
> +};
Ok, I'm getting happier. This time I looked at what locks we were
holding for accessing state_default and legacy_userspace_busy. I'm
afraid we alternated between no locking and struct_mutex. This will add
some unfortunate complexity...
> +
> typedef struct drm_i915_private {
> struct drm_device *dev;
> struct kmem_cache *slab;
> @@ -1418,6 +1424,8 @@ typedef struct drm_i915_private {
>
> struct i915_package_c8 pc8;
>
> + struct i915_gt_slices gt_slices;
> +
> /* Old dri1 support infrastructure, beware the dragons ya fools entering
> * here! */
> struct i915_dri1_state dri1;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0ce0d47..7a362a2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -922,6 +922,62 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
> return 0;
> }
>
> +static int gt_legacy_userspace_busy(struct intel_ring_buffer *ring)
> +{
> + int ret;
> +
if (dev_priv->gt_slices.state_default == 1)
return 0;
> + ret = intel_ring_begin(ring, 18);
> + if (ret)
> + return ret;
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, HSW_GT_SLICE_INFO);
> + intel_ring_emit(ring, SLICE_SEL_BOTH);
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, MI_PREDICATE_RESULT_2);
> + intel_ring_emit(ring, LOWER_SLICE_ENABLED);
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, HSW_SLICESHUTDOWN);
> + intel_ring_emit(ring, ~SLICE_SHUTDOWN);
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, RC_IDLE_MAX_COUNT);
> + intel_ring_emit(ring, CS_IDLE_COUNT_1US);
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, WAIT_FOR_RC6_EXIT);
> + intel_ring_emit(ring, _MASKED_BIT_ENABLE(WAIT_RC6_EXIT));
> +
> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> + intel_ring_emit(ring, RC_IDLE_MAX_COUNT);
> + intel_ring_emit(ring, CS_IDLE_COUNT_5US);
> +
> + intel_ring_advance(ring);
> + return 0;
> +}
> +
> +static bool gt_legacy_userspace(struct intel_ring_buffer *ring,
> + struct drm_i915_gem_execbuffer2 *args)
> +{
> + drm_i915_private_t *dev_priv = ring->dev->dev_private;
> +
> + if (ring->id == BCS)
> + return false;
> +
> + if (!HAS_SLICE_SHUTDOWN(ring->dev))
> + return false;
> +
> + if (dev_priv->gt_slices.state_default == 1)
> + return false;
> +
> + if (dev_priv->gt_slices.legacy_userspace_busy)
> + return false;
Remove these two.
> +
> + return (args->flags & I915_EXEC_USE_PREDICATE) == 0;
> +}
> +
> static int
> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct drm_file *file,
> @@ -999,6 +1055,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> return -EINVAL;
> }
>
> + if (gt_legacy_userspace(ring, args)) {
mutex_lock(&dev_priv->gt_slices.lock);
if (!dev_priv->gt_slices.legacy_userspace_busy) {
> + ret = gt_legacy_userspace_busy(ring);
> + if (ret == 0)
> + dev_priv->gt_slices.legacy_userspace_busy = true;
}
mutex_unlock(&dev_priv->gt_slices.lock);
if (ret)
return ret;
> + }
Or you can even push that locking and conditional down to
gt_legacy_userspace_busy().
Fixing the use of dev_priv->gt_slices.lock around the other instances
of gt_slices.legacy_userspace_busy and gt_slices.state_default should be
easier.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
