[Intel-gfx] [PATCH 1/7] drm/i915: Record GT workarounds in a list
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Dec 3 12:34:24 UTC 2018
On 03/12/2018 11:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-12-03 11:46:11)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> To enable later verification of GT workaround state at various stages of
>> driver lifetime, we record the list of applicable ones per platforms to a
>> list, from which they are also applied.
>>
>> The added data structure is a simple array of register, mask and value
>> items, which is allocated on demand as workarounds are added to the list.
>>
>> This is a temporary implementation which later in the series gets fused
>> with the existing per context workaround list handling. It is separated at
>> this stage since the following patch fixes a bug which needs to be as easy
>> to backport as possible.
>>
>> Also, since in the following patch we will be adding a new class of
>> workarounds (per engine) which can be applied from interrupt context, we
>> straight away make the provision for safe read-modify-write cycle.
>>
>> v2:
>> * Change dev_priv to i915 along the init path. (Chris Wilson)
>> * API rename. (Chris Wilson)
>>
>> v3:
>> * Remove explicit list size tracking in favour of growing the allocation
>> in power of two chunks. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk> # v2
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 1 +
>> drivers/gpu/drm/i915/i915_drv.h | 2 +
>> drivers/gpu/drm/i915/i915_gem.c | 4 +-
>> drivers/gpu/drm/i915/intel_workarounds.c | 491 +++++++++++++++--------
>> drivers/gpu/drm/i915/intel_workarounds.h | 23 +-
>> 5 files changed, 353 insertions(+), 168 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e39016713464..ee5116b62cd2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1453,6 +1453,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
>>
>> intel_uncore_sanitize(dev_priv);
>>
>> + intel_gt_init_workarounds(dev_priv);
>> i915_gem_load_init_fences(dev_priv);
>>
>> /* On the 945G/GM, the chipset reports the MSI capability on the
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 43ac6873a2bb..9ddbcc1f3554 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -69,6 +69,7 @@
>> #include "intel_ringbuffer.h"
>> #include "intel_uncore.h"
>> #include "intel_wopcm.h"
>> +#include "intel_workarounds.h"
>> #include "intel_uc.h"
>>
>> #include "i915_gem.h"
>> @@ -1652,6 +1653,7 @@ struct drm_i915_private {
>> int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
>>
>> struct i915_workarounds workarounds;
>> + struct i915_wa_list gt_wa_list;
>>
>> struct i915_frontbuffer_tracking fb_tracking;
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index c55b1f75c980..6333a7d6af5a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -5334,7 +5334,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>> I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ?
>> LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED);
>>
>> - intel_gt_workarounds_apply(dev_priv);
>> + intel_gt_apply_workarounds(dev_priv);
>>
>> i915_gem_init_swizzling(dev_priv);
>>
>> @@ -5706,6 +5706,8 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>> i915_gem_contexts_fini(dev_priv);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>>
>> + intel_wa_list_free(&dev_priv->gt_wa_list);
>> +
>> intel_cleanup_gt_powersave(dev_priv);
>>
>> intel_uc_fini_misc(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
>> index e5cd6c6c66c3..f05f13547034 100644
>> --- a/drivers/gpu/drm/i915/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
>> @@ -48,6 +48,18 @@
>> * - Public functions to init or apply the given workaround type.
>> */
>>
>> +static void wa_init_start(struct i915_wa_list *wal, const char *name)
>> +{
>> + wal->name = name;
>> +}
>> +
>> +static void wa_init_finish(struct i915_wa_list *wal)
>> +{
>> + if (wal->count)
>> + DRM_DEBUG_DRIVER("Initialized %u %s workarounds\n",
>> + wal->count, wal->name);
>
> Does this become if (!wal->count) return; later?
Done.
>> +}
>> +
>> static void wa_add(struct drm_i915_private *i915,
>> i915_reg_t reg, const u32 mask, const u32 val)
>> {
>> @@ -575,160 +587,240 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
>> return 0;
>> }
>>
>> -static void bdw_gt_workarounds_apply(struct drm_i915_private *dev_priv)
>> +static void
>> +wal_add(struct i915_wa_list *wal, const struct i915_wa *wa)
>> +{
>> + const unsigned int grow = 1 << 4;
>> +
>> + GEM_BUG_ON(!is_power_of_2(grow));
>> +
>> + if (IS_ALIGNED(wal->count, grow)) { /* Either uninitialized or full. */
>
> Neat.
>
>> + struct i915_wa *list;
>> +
>> + list = kcalloc(ALIGN(wal->count + 1, grow), sizeof(*wa),
>> + GFP_KERNEL);
>
> (Quietly comments on the calloc here ;)
Oh I don't want to complicate things with zeroing the tail. Or you
wouldn't bother with zeroing at all since I always copy over used
entries? So unused, who cares about them?
>
>> + if (!list) {
>> + DRM_ERROR("No space for workaround init!\n");
>> + return;
>> + }
>> +
>> + if (wal->list)
>> + memcpy(list, wal->list, sizeof(*wa) * wal->count);
>> +
>> + wal->list = list;
>> + }
>> +
>> + memcpy(&wal->list[wal->count], wa, sizeof(*wa));
>> + wal->count++;
>
> I'd push for
> wal->list[wal->count++] = wa;
> Might as well use static type checking.
Ok.
>> +struct i915_wa_list {
>> + const char *name;
>> + unsigned int count;
>> + struct i915_wa *list;
>
> Oh well, didn't save anything after all.
How nothing, one unsigned int per wa_list instance! :)
Regards,
Tvrtko
More information about the Intel-gfx
mailing list