| Message ID | 20201010002105.45152-5-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow privileged user to map the OA buffer | expand |
Quoting Umesh Nerlige Ramappa (2020-10-10 01:21:03) > Switch the search and grow code of the _wa_add to use _wa_index and > _wa_list_grow. > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 +++++++-------------- > 1 file changed, 17 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 801fcb60f46b..e49283bffa33 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -174,53 +174,33 @@ static void _wa_remove(struct i915_wa_list *wal, i915_reg_t reg, u32 flags) > > static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa) > { > - unsigned int addr = i915_mmio_reg_offset(wa->reg); > - unsigned int start = 0, end = wal->count; > + int index; > const unsigned int grow = WA_LIST_CHUNK; > struct i915_wa *wa_; > > GEM_BUG_ON(!is_power_of_2(grow)); > > - if (IS_ALIGNED(wal->count, grow)) { /* Either uninitialized or full. */ > - struct i915_wa *list; > - > - list = kmalloc_array(ALIGN(wal->count + 1, grow), sizeof(*wa), > - GFP_KERNEL); > - if (!list) { > - DRM_ERROR("No space for workaround init!\n"); > + if (IS_ALIGNED(wal->count, grow)) /* Either uninitialized or full. */ > + if (_wa_list_grow(wal, wal->count) < 0) > return; > - } > - > - if (wal->list) > - memcpy(list, wal->list, sizeof(*wa) * wal->count); > > - wal->list = list; An inherited problem, but I'm a little unnerved by the apparent leak of wa->list here. Paging Tvrtko to see if he can remember if there's a hidden trick. -Chris --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 12/11/2020 19:44, Chris Wilson wrote: > Quoting Umesh Nerlige Ramappa (2020-10-10 01:21:03) >> Switch the search and grow code of the _wa_add to use _wa_index and >> _wa_list_grow. >> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 +++++++-------------- >> 1 file changed, 17 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> index 801fcb60f46b..e49283bffa33 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c >> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> @@ -174,53 +174,33 @@ static void _wa_remove(struct i915_wa_list *wal, i915_reg_t reg, u32 flags) >> >> static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa) >> { >> - unsigned int addr = i915_mmio_reg_offset(wa->reg); >> - unsigned int start = 0, end = wal->count; >> + int index; >> const unsigned int grow = WA_LIST_CHUNK; >> struct i915_wa *wa_; >> >> GEM_BUG_ON(!is_power_of_2(grow)); >> >> - if (IS_ALIGNED(wal->count, grow)) { /* Either uninitialized or full. */ >> - struct i915_wa *list; >> - >> - list = kmalloc_array(ALIGN(wal->count + 1, grow), sizeof(*wa), >> - GFP_KERNEL); >> - if (!list) { >> - DRM_ERROR("No space for workaround init!\n"); >> + if (IS_ALIGNED(wal->count, grow)) /* Either uninitialized or full. */ >> + if (_wa_list_grow(wal, wal->count) < 0) >> return; >> - } >> - >> - if (wal->list) >> - memcpy(list, wal->list, sizeof(*wa) * wal->count); >> >> - wal->list = list; > > An inherited problem, but I'm a little unnerved by the apparent leak of > wa->list here. > > Paging Tvrtko to see if he can remember if there's a hidden trick. I don't see any tricks, just a memory leak for all lists with more than 16 unique registers on the wa list. :( Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-11-13 10:40:07) > > On 12/11/2020 19:44, Chris Wilson wrote: > > Quoting Umesh Nerlige Ramappa (2020-10-10 01:21:03) > > An inherited problem, but I'm a little unnerved by the apparent leak of > > wa->list here. > > > > Paging Tvrtko to see if he can remember if there's a hidden trick. > > I don't see any tricks, just a memory leak for all lists with more than > 16 unique registers on the wa list. :( As I read it, since each pair of perf open/close will cause us to reallocate the lists, we need to address this leak first before it becomes user controllable. Apologies for yet another delay. -Chris --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index 801fcb60f46b..e49283bffa33 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -174,53 +174,33 @@ static void _wa_remove(struct i915_wa_list *wal, i915_reg_t reg, u32 flags) static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa) { - unsigned int addr = i915_mmio_reg_offset(wa->reg); - unsigned int start = 0, end = wal->count; + int index; const unsigned int grow = WA_LIST_CHUNK; struct i915_wa *wa_; GEM_BUG_ON(!is_power_of_2(grow)); - if (IS_ALIGNED(wal->count, grow)) { /* Either uninitialized or full. */ - struct i915_wa *list; - - list = kmalloc_array(ALIGN(wal->count + 1, grow), sizeof(*wa), - GFP_KERNEL); - if (!list) { - DRM_ERROR("No space for workaround init!\n"); + if (IS_ALIGNED(wal->count, grow)) /* Either uninitialized or full. */ + if (_wa_list_grow(wal, wal->count) < 0) return; - } - - if (wal->list) - memcpy(list, wal->list, sizeof(*wa) * wal->count); - wal->list = list; - } + index = _wa_index(wal, wa->reg); + if (index >= 0) { + wa_ = &wal->list[index]; - while (start < end) { - unsigned int mid = start + (end - start) / 2; + if ((wa->clr | wa_->clr) && !(wa->clr & ~wa_->clr)) { + DRM_ERROR("Discarding overwritten w/a for reg %04x (clear: %08x, set: %08x)\n", + i915_mmio_reg_offset(wa_->reg), + wa_->clr, wa_->set); - if (i915_mmio_reg_offset(wal->list[mid].reg) < addr) { - start = mid + 1; - } else if (i915_mmio_reg_offset(wal->list[mid].reg) > addr) { - end = mid; - } else { - wa_ = &wal->list[mid]; - - if ((wa->clr | wa_->clr) && !(wa->clr & ~wa_->clr)) { - DRM_ERROR("Discarding overwritten w/a for reg %04x (clear: %08x, set: %08x)\n", - i915_mmio_reg_offset(wa_->reg), - wa_->clr, wa_->set); - - wa_->set &= ~wa->clr; - } - - wal->wa_count++; - wa_->set |= wa->set; - wa_->clr |= wa->clr; - wa_->read |= wa->read; - return; + wa_->set &= ~wa->clr; } + + wal->wa_count++; + wa_->set |= wa->set; + wa_->clr |= wa->clr; + wa_->read |= wa->read; + return; } wal->wa_count++;
Switch the search and grow code of the _wa_add to use _wa_index and _wa_list_grow. Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 +++++++-------------- 1 file changed, 17 insertions(+), 37 deletions(-)