Re: [Intel-gfx] [PATCH 1/2] drm/i915/gt: Add GT oriented dmesg output

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

 



On 11/10/2022 02:33, Jani Nikula wrote:
On Wed, 09 Nov 2022, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote:
Instead of merging this patch now, oriented on GT only, I would rather
wait until we discuss and plan solution for the all sub-components.

Once that's done (with agreement on naming and output) we can start
converting exiting messages.

My proposal would be:
  - use wrappers per component
  - use lower case names
  - don't add colon

#define i915_xxx(_i915, _fmt, ...) \
	drm_xxx(&(_i915)->drm, _fmt, ##__VA_ARGS__)

#define gt_xxx(_gt, _fmt, ...) \
	i915_xxx((_gt)->i915, "GT%u " _fmt, (_gt)->info.id, ..

#define guc_xxx(_guc, _fmt, ...) \
	gt_xxx(guc_to_gt(_guc), "GuC " _fmt, ..

#define ct_xxx(_ct, _fmt, ...) \
	guc_xxx(ct_to_guc(_ct), "CTB " _fmt, ..

where
	xxx = { err, warn, notice, info, dbg }

and then for calls like:

	i915_err(i915, "Foo failed (%pe)\n", ERR_PTR(err));
	  gt_err(gt,   "Foo failed (%pe)\n", ERR_PTR(err));
	 guc_err(guc,  "Foo failed (%pe)\n", ERR_PTR(err));
	  ct_err(ct,   "Foo failed (%pe)\n", ERR_PTR(err));

the output would look like:

[ ] i915 0000:00:02.0: [drm] *ERROR* Foo failed (-EIO)
[ ] i915 0000:00:02.0: [drm] *ERROR* GT0 Foo failed (-EIO)
[ ] i915 0000:00:02.0: [drm] *ERROR* GT0 GuC Foo failed (-EIO)
[ ] i915 0000:00:02.0: [drm] *ERROR* GT0 GuC CTB Foo failed (-EIO)

Would that work for all of you?
I think we borderline have too many wrappers already. I've basically
blocked the i915_* variants early on in favour of just using the drm_*
calls directly - which, themselves, are also wrappers on dev_*.

In general, there's aversion in the kernel to adding wrappers that hide
what's actually going on. WYSIWYG.
That is blatantly not the case. There is a very strong push in kernel code reviews to abstract common code into helper functions. How is this any different? If you removed all helper functions with the desire to have truly WYSIWYG code then each line of code would be a paragraph long.


So I wasn't thrilled to see the GT_* wrappers either.

And indeed part of it is that anything you do sets an example for the
next developer coming along. You add wrappers for this, and sure enough
someone's going to cargo cult and add wrappers for some other
component. We didn't even have to merge this to see that idea pop up.

The thing with logging is that it's not just one wrapper. It's info,
notice, warn, err, multiplied by the "once" variants, with some
duplicated by the "ratelimited" variants. (And yeah, C sucks at this.)

Arguably the display code could use logging wrappers for connectors,
encoders and crtcs, too. Not just i915, but across drm. See:

git grep "\[\(CONNECTOR\|ENCODER\|CRTC\)"

But we've opted not to.

This is just my educated guess, but what I think is more likely to
happen is adding a "debug name" member to the relevant structs that you
could use like:

	drm_dbg_kms(connector->dev, "%s foo\n", connector->dbg_id);
Huh? You want the wrapper to be called drm_dbg_gt instead of gt_dbg but otherwise it is identical to what is currently in the patch? Other than being a longer name, how is that any different? It is still very much a wrapper of exactly the type that you don't seem to like? And as noted already, in that case shouldn't we be pushing a patch to rename drm_dbg to dev_dbg_drm first? In which case the above should be dev_dbg_drm_kms?



---

I'm not going to block this patchset, but I also really *really* don't
want to see a cargo culted copy of this for other components. Which kind
of makes me anxious.
I really don't understand the problem with adding such wrappers. Sure, there are a bunch of varients - dbg, err, warn, etc. But the variant list is contained and unlikely to grow at any point. It is trivially easy to look up gt_dbg to find out what it is doing. It makes the code much easier to read. I mean, who cares that gt_dbg is implemented via drm_dbg which is implemented via dev_dbg? Only someone that is actively wanting to further change the implementation. Anyone who is using it just wants to know that they are calling the correct debug printer for their module and are giving it the correct parameters. Having a nice simple name makes that totally clear.



So what if you looked again what you could do to make using the drm_*
logging variants easier?
Not a lot. All other proposals so far have been adding complicated and unwieldy constructs to every single print.

Fundamentally, the drm_dbg wrappers as they stand do not allow for further customisation. So your choice is to replace, rewrite or wrap. Replacing it with an entirely parallel implementation is fundamentally the wrong direction. Re-writing the DRM layer to take a pointer to a custom printer object instead of a 'struct drm_device' (where each sub-module defines it's own private printer that adds it's preferred string and chains on to the next printer) seems like a huge change to the entire DRM code base for very little benefit. The option of wrapping the just arguments makes for messy code. The option of wrapping the function call itself means wrapping five functions. Pick your poison.

John.




BR,
Jani.







[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux