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.
