Re: [PATCH 3/4] drm/i915: HSW GT3 Slices: exec flag to warn kernel that userspace is using predication

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Chris,

Thank you very much for all suggestions. Will do the changes. But for
that question below I don't have the clear answer:

On Wed, Oct 16, 2013 at 6:15 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 15, 2013 at 05:50:49PM -0300, 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.
>>
>> 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 | 67 ++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_reg.h            | 11 +++++
>>  drivers/gpu/drm/i915/i915_sysfs.c          | 11 ++++-
>>  drivers/gpu/drm/i915/intel_display.c       |  6 +++
>>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>>  drivers/gpu/drm/i915/intel_pm.c            | 39 ++++++++++++++++-
>>  include/uapi/drm/i915_drm.h                |  8 +++-
>>  8 files changed, 146 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 685fb1d..bd4774e 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;
>
> Either use it as an integer (a count of slices enabled by default) or
> make it an enum.
>
>> +     int forcing_full;
>
> bool legacy_userspace_busy; ?
>
>> +     struct mutex m_lock;
>
> Ugh. Just struct mutex lock; /* locks all access to this struct and
> slice registers */ will suffice
>
>> +};
>> +
>>  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..eb09a51 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -923,6 +923,66 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>>  }
>>
>>  static int
>> +i915_gt_full_immediate(struct drm_device *dev, struct intel_ring_buffer *ring)
>
> That's a misnomer.
>
>> +{
>> +     drm_i915_private_t *dev_priv = dev->dev_private;
>> +     int ret;
>> +
>> +     if (!HAS_SLICE_SHUTDOWN(dev) || ring == &dev_priv->ring[BCS])
>> +             return 0;
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     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_advance(ring);
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     if (ret)
>> +             return ret;
>> +     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_advance(ring);
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     if (ret)
>> +             return ret;
>> +     intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
>> +     intel_ring_emit(ring, HSW_SLICESHUTDOWN);
>> +     intel_ring_emit(ring, ~SLICE_SHUTDOWN);
>> +     intel_ring_advance(ring);
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     if (ret)
>> +             return ret;
>> +     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_advance(ring);
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     if (ret)
>> +             return ret;
>> +     intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
>> +     intel_ring_emit(ring, WAIT_FOR_RC6_EXIT);
>> +     intel_ring_emit(ring, WAIT_RC6_EXIT | MASK_WAIT_RC6_EXIT);
>> +     intel_ring_advance(ring);
>> +
>> +     ret = intel_ring_begin(ring, 3);
>> +     if (ret)
>> +             return ret;
>> +     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 int
>>  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>                      struct drm_file *file,
>>                      struct drm_i915_gem_execbuffer2 *args,
>> @@ -999,6 +1059,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>               return -EINVAL;
>>       }
>>
>> +     if ((args->flags & I915_EXEC_USE_PREDICATE) == 0 &&
>> +         dev_priv->gt_slices.state_default == 0 &&
>> +         !dev_priv->gt_slices.forcing_full) {
>
> Always set legacy_userspace_busy here so that a change in state_default
> cannot possibly break currently active users i.e. they are independent
> bookkeeping.
>
>> +             dev_priv->gt_slices.forcing_full = true;
>> +             i915_gt_full_immediate(dev, ring);
>
> What happens when this fails? If it only partially emits the commands,
> etc.

This is a very good question. If it fails, my only concern would be
the sync between HSW_GT_SLICE_INFO and MI_PREDICATE_RESULT_2. But I
don't know how to do this during the exec buf without delaying the
execution. Do you have any suggestion?

>
>> +     }
>> +
>>       mode = args->flags & I915_EXEC_CONSTANTS_MASK;
>>       mask = I915_EXEC_CONSTANTS_MASK;
>>       switch (mode) {
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 497c441..0146bef 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -277,6 +277,17 @@
>>  #define   SLICE_STATUS_MAIN_ON       (2<<0)
>>  #define   SLICE_STATUS_BOTH_ON       (3<<0)
>>
>> +#define HSW_SLICESHUTDOWN    0xA190
>> +#define   SLICE_SHUTDOWN     (1<<0)
>> +
>> +#define RC_IDLE_MAX_COUNT    0x2054
>> +#define   CS_IDLE_COUNT_1US  (1<<1)
>> +#define   CS_IDLE_COUNT_5US  (1<<3)
>> +
>> +#define WAIT_FOR_RC6_EXIT    0x20CC
>> +#define   WAIT_RC6_EXIT              (1<<0)
>> +#define   MASK_WAIT_RC6_EXIT (1<<16)
>> +
>>  /*
>>   * 3D instructions used by the kernel
>>   */
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> index 86ccd52..25b0c5b 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -124,9 +124,13 @@ static ssize_t gt_slice_config_show(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;
>> +     bool full;
>>
>> -     return sprintf(buf, "%s\n", I915_READ(MI_PREDICATE_RESULT_2) ==
>> -                    LOWER_SLICE_ENABLED ? "full" : "half");
>> +     mutex_lock(&dev_priv->gt_slices.m_lock);
>> +     full = I915_READ(MI_PREDICATE_RESULT_2) == LOWER_SLICE_ENABLED;
>> +     mutex_unlock(&dev_priv->gt_slices.m_lock);
>
> This locking is not stopping any races - it is superfluous.
>
>> +     return sprintf(buf, "%s\n", full ? "full" : "half");
>>  }
>>
>>  static ssize_t gt_slice_config_store(struct device *kdev,
>> @@ -135,16 +139,19 @@ 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;
>> +             dev_priv->gt_slices.state_default = 1;
>>       } else if (!strncmp(buf, "half", sizeof("half") - 1)) {
>>               ret = intel_set_gt_half(dev);
>>               if (ret)
>>                       return ret;
>> +             dev_priv->gt_slices.state_default = 0;
>>       } else
>>               return -EINVAL;
>>       return count;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4f1b636..1a2f8bc 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -7778,6 +7778,12 @@ void intel_mark_idle(struct drm_device *dev)
>>
>>       if (dev_priv->info->gen >= 6)
>>               gen6_rps_idle(dev->dev_private);
>> +
>> +     if (HAS_SLICE_SHUTDOWN(dev) && dev_priv->gt_slices.forcing_full &&
>> +         dev_priv->gt_slices.state_default == 0) {
>> +             intel_set_gt_half_async(dev);
>> +             dev_priv->gt_slices.forcing_full = false;
>
> Again, you want to be ignoring state_default when changing
> legacy_userspace_busy.
>
> The lower level slice config handling code should be making the judgment
> based on all parameters after one changes.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux