Hi Jeff,
> On Mon, 2025-06-23 at 14:01 +0000, Krzysztof Karas wrote:
> > Hi Jeff,
> >
> > [...]
> > > +static __maybe_unused int
> > > +ref_tracker_dir_seq_print(struct ref_tracker_dir *dir, struct seq_file
> > > *seq)
> > >
Hi Jeff,
[...]
> +static __maybe_unused int
> +ref_tracker_dir_seq_print(struct ref_tracker_dir *dir, struct seq_file *seq)
> +{
> + struct ostream os = { .func = pr_ostream_seq,
> + .prefix = "",
This is also a "ref_tracker_*" function, so maybe use the same
prefix a
char *class; /* object classname */
Another nitpick here: the comments start with capital letters in
this struct, so could you change object -> Object?
Apart from these minor suggestions, I think this is a nice
change:
Reviewed-by: Krzysztof Karas
---
Best Regards,
Krzysztof
+), 15 deletions(-)
Looks good to me:
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
x27;s too late for the
> pr_fmt macro.
>
> Reviewed-by: Andrew Lunn
> Signed-off-by: Jeff Layton
> ---
Looks good:
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
ed-by: Andrew Lunn
> Signed-off-by: Jeff Layton
> ---
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
er is still set to NULL. CI has shown that the
> ref_tracker_dir can be initialized more than once.
>
> Signed-off-by: Jeff Layton
> ---
Looks good:
Reviewed-by: Krzysztof Karas
A side note:
I was wondering if we could improve naming a bit, but I think it
is not worth the e
Hi Janusz,
[...]
> If successful then that function, or its execlists or GuC submission
> equivalent, is supposed to be called only once per GEM context engine,
Could you clarify "execlists or GuC submission equivalent" here
- do these functions perform similar reference acquisition,
which may be
Hi Jeff,
> Add a new "ref_tracker" directory in debugfs. Each individual refcount
> tracker can register files under there to display info about
> currently-held references.
>
> Reviewed-by: Andrew Lunn
> Signed-off-by: Jeff Layton
> ---
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
Hi Jeff,
[...]
> +static void __ostream_printf pr_ostream_buf(struct ostream *stream, char
> *fmt, ...)
> +{
> + int ret, len = stream->size - stream->used;
> + va_list args;
> +
> + va_start(args, fmt);
> + ret = vsnprintf(stream->buf + stream->used, len, fmt, args);
vsnprintf()
> [1]:
> https://lore.kernel.org/netdev/20250414-restricted-pointers-net-v1-0-12af0ce46...@linutronix.de/
>
> Cc: Thomas Weißschuh
> Reviewed-by: Thomas Weißschuh
> Signed-off-by: Jeff Layton
> ---
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
Hi Jeff,
> I got some warnings from the i915 CI with the ref_tracker debugfs
> patches applied, that indicated that these ref_tracker_dir_init() calls
> were being called more than once. If references were held on these
> objects between the initializations, then that could lead to leaked ref
> tr
Hi Krzysztof,
> Introduce a CAP_PERFMON check when accessing sysfs entries related to
> local memory information. Also introduce a intel_memory_info_paranoid
> sysctl parameter, which allows the administrator to control whether the
> check is enforced.
If we decide that this patch is neede, I thin
Hi Krzysztof,
> This series introduces a way for applications to read local memory
> information via files in the sysfs. So far the only way to do this was
> via i915_query ioctl. This is slightly less handy than sysfs for
> external users.
"So far the only way to do this was via i915_query ioctl
Hi Krzysztof,
[...]
> +static ssize_t
> +vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + struct device *dev = kobj_to_dev(kobj->parent);
> + struct intel_memory_region *mr;
> +
> + mr = intel_memory_region_by_type(kdev_minor_to_i915(dev),
> INTE
vcoredump have more information about what happened
> - Update that PID and APP will be empty if there's no app info
> ---
> Documentation/gpu/drm-uapi.rst | 17 +
LGTM:
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
was this PID's name,
> so to make the life easier also notify what's the app's name in the user
> event.
>
> Acked-by: Rodrigo Vivi (for i915 and xe)
> Signed-off-by: André Almeida
> ---
> v3: Make comm_string and pid_string empty when there's no app info
LGTM:
Reviewed-by: Krzysztof Karas
Best Regards,
Krzysztof
Hi André,
[...]
> @@ -582,6 +584,14 @@ int drm_dev_wedged_event(struct drm_device *dev,
> unsigned long method)
> drm_info(dev, "device wedged, %s\n", method == DRM_WEDGE_RECOVERY_NONE ?
>"but recovered through reset" : "needs recovery");
>
> + if (info) {
> +
aniele Ceraolo Spurio
> Cc: Alan Previn
> Signed-off-by: Janusz Krzysztofik
I have tested this on a kernel build with kconfig from one of
the failing runs. Before your patch the reproduction came quite
soon after a few dozen of runs, but I have been testing this now
for a prolonged time and d
Hi Janusz,
thanks for a quick response.
> > throughout the series you modify the code right after
> > introducing it.
>
> Yes, that split among patches reflects my way of getting to a solution that
> not only resolves the issue but also tries to address comments I got and take
> care of resul
Hi Janusz,
throughout the series you modify the code right after
introducing it. How about changing the order of things a bit:
1) order the functions in a symmetrical way between
register/unregister steps and group them as you see necessary,
(At that point you would not be fixing the issue ye
uot;)
Signed-off-by: Krzysztof Karas
Reviewed-by: Mikolaj Wasiak
---
Changelog:
* v1 -> v2:
Avoid calling i915_gem_ww_ctx_fini() with zeroed context
by returning early (Sebastian).
* v2 -> v3:
Use an additional label for clenup path (Mikolaj).
drivers/
Hi Mikolaj,
> > + if (!ppgtt->vm.allocate_va_range) {
> > + i915_vm_put(&ppgtt->vm);
> > + return 0;
> > + }
>
> I don't know if it feels more in line with kernel style, but consider
> changing it to a label before second `i915_vm_put` at end of function
> plus goto instea
I sent an earlier version of this patch by mistake (it includes
indentation errors) - I apologize for that, will fix this in v3
later. Sorry for the noise.
Krzysztof
On 2025-01-28 at 12:53:44 +, Krzysztof Karas wrote:
> There is an error path in igt_ppgtt_alloc(), which leads to ww
>
;)
Signed-off-by: Krzysztof Karas
---
Changelog:
* v1 -> v2:
Avoid calling i915_gem_ww_ctx_fini() with zeroed context
by returning early (Sebastian).
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu
Hi Sebastian,
thanks for review.
> I don't thing it's a best idea to just initialize ww here, you still have
> incorrect path that try to fini ww before it was initialize.
Fair point - we still call i915_gem_ww_ctx_fini(), which is
useless without actual ww.
>
> I would probably do something lik
Hi Maciej,
> The locked==true looks OK.
>
thanks for review!
> However, I fear that there is some corner case with locked==false. 1 or 2
> calls back in chain looks good.
>
> CI failures needs to be analyzed.
>
Yup, I already did that recently. I thought those were
suspicious, but I could not r
There is an error path in igt_ppgtt_alloc(), which leads to ww
object being passed down to i915_gem_ww_ctx_fini() without
initialization. Correct that by zeroing the struct.
Fixes: 480ae79537b2 ("drm/i915/selftests: Prepare gtt tests for obj->mm.lock
removal")
Signed-off-by: Kr
Hi Ingyu,
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index d60a6ca0cae5..8d22d8f2243d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_b
o what we asked for which was ". That seems
> like the logical order and description to me. The line is already fairly
> long so the idea was not to make it any longer than necessary while still
> giving all the useful information in a sensible manner.
Fair enough. If the line length is a concern, then there is not
much that can be done. A good addition overall.
Reviewed-by: Krzysztof Karas
Krzysztof
Hi John,
> From: John Harrison
>
> To aid debug of sporadic issues, include the requested frequency in
> the debug message as well as the actual frequency. That way we know
> for certain that the clamping is not because the driver forgot to ask.
>
...
> } else if (delta_ms > 200) {
>
ff-by: Krzysztof Karas
Fixes: 2f2cc53b5fe7 ("drm/i915/guc: Close deregister-context race against
CT-loss")
Cc: # v6.9+
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submissi
Hi Tvrtko,
> On 14/01/2025 09:00, Krzysztof Karas wrote:
> > spin_unlock() function enables irqs regardless of their state
>
> It doesn't, you confuse spin_unlock with spin_unlock_irq.
>
> > before spin_lock() was called. This might result in an interrupt
> >
irqs' state save/restore calls to all locks/unlocks in
signal_irq_work() execution (Maciej)
Signed-off-by: Krzysztof Karas
---
This issue is hit rarely on CI and I was not able to reproduce
it locally. There might be more places where we should save and
restore irq state, so I am not a
Hi Maciej,
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 12f1ba7ca9c1..e9102f7246f5 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
vaddr = kmap(page);
> + vaddr = kmap_local_page(page);
> iosys_map_memcpy_to(map, map_off, vaddr + offset_in_page(off),
> this);
> mark_page_accessed(page);
> - kunmap(page);
> + kunmap_local(vaddr);
> put_page(page);
>
> len -= this;
> --
Reviewed-by: Krzysztof Karas
Krzysztof
>
flag is
handled.
Signed-off-by: Krzysztof Karas
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c| 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 127 ++
3 files changed, 74 insertions(+), 67 deletions(-)
diff --git a/
-off-by: Krzysztof Karas
---
This issue is hit rarely on CI and I was not able to reproduce
it locally. There might be more places where we should save and
restore irq state, so I am not adding "Closes" label for the
issue yet.
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +++
ug.
Having a test for that would be beneficial - it would let us verify the
necessity of this commit. Looking forward to it :)
> I will add Fixes tag.
Please do, having a reference point would be good.
After you add that:
Reviewed-by: Krzysztof Karas
Krzysztof
Hi Jacek,
On 2025-01-08 at 11:53:46 +0100, Jacek Lawrynowicz wrote:
> drm_gem_mmap_obj() expects VM_DONTEXPAND flag to be set after mmap
> callback is executed. Set this flag at the end of i915_gem_dmabuf_mmap()
> to prevent WARN on mmap in buffers imported from i915 e.g.,
...
> +++ b/drivers/gpu/
nable_interrupts' not found
>
Hi Harshit Mogalapalli,
I confirmed that the patch fixes these warnings.
Reviewed-by: Krzysztof Karas
Krzysztof Karas
> intel_runtime_pm_disable_interrupts() is renamed to intel_irq_suspend(),
> make documentation changes accordingly.
>
> Fixe
41 matches
Mail list logo
