On Fri, Nov 1, 2013 at 8:39 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> 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.
What else is wrong besides what you pointed on sysfs?
>
>> @@ -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.
I used 0 for half and 1 for full like I used on the boot parameter.
I don't mind in changing that if you believe 1 and 2 is more clear...
but I would have to change all other patches and maybe the sysfs
interface?
>
>> + 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?
agree... I will just remove it, but what else you see wrong with locking?
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Thanks,
Rodrigo.
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
