On Thu, Oct 31, 2013 at 09:07:09PM -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.
>
> v5: fix mutex_lock use by Chris.
>
> CC: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> CC: Eric Anholt <eric@xxxxxxxxxx>
> CC: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Locking needs major work still.
> @@ -935,6 +985,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> struct drm_clip_rect *cliprects = NULL;
> struct intel_ring_buffer *ring;
> struct i915_ctx_hang_stats *hs;
> + struct i915_gt_slices *gt_slices = &dev_priv->gt_slices;
> u32 ctx_id = i915_execbuffer2_get_context_id(*args);
> u32 exec_start, exec_len;
> u32 mask, flags;
> @@ -999,6 +1050,19 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> return -EINVAL;
> }
>
> + if (gt_legacy_userspace(ring, args)) {
> + mutex_lock(>_slices->lock);
> + if (gt_slices->state_default == 0 &&
> + !gt_slices->legacy_userspace_busy) {
You need to set legacy_userspace_busy if gt_slices->state_default is
already 0. Why 0? Why not 1? 1 - for one slice, 2 - for two slices, etc.
> + ret = gt_legacy_userspace_busy(ring);
> + if (ret == 0)
> + gt_slices->legacy_userspace_busy = true;
> + }
> + mutex_unlock(>_slices->lock);
> + if (ret)
> + return ret;
> + }
> +
> mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> mask = I915_EXEC_CONSTANTS_MASK;
> switch (mode) {
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 86ccd52..a821499 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -135,16 +135,23 @@ static ssize_t gt_slice_config_store(struct device *kdev,
> {
> struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> struct drm_device *dev = minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
>
> if (!strncmp(buf, "full", sizeof("full") - 1)) {
> ret = intel_set_gt_full(dev);
> if (ret)
> return ret;
> + mutex_lock(&dev_priv->gt_slices.lock);
> + dev_priv->gt_slices.state_default = 1;
> + mutex_unlock(&dev_priv->gt_slices.lock);
> } else if (!strncmp(buf, "half", sizeof("half") - 1)) {
> ret = intel_set_gt_half(dev);
> if (ret)
> return ret;
> + mutex_lock(&dev_priv->gt_slices.lock);
> + dev_priv->gt_slices.state_default = 0;
> + mutex_unlock(&dev_priv->gt_slices.lock);
> } else
> return -EINVAL;
This is the clearest example that the locking is fubar. Consider a
second process that simultaneously tries to change slice config. What
state is recorded? What state is the hardware actually in?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
