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

[John Harrison]

On 11/10/2022 02:33, Jani Nikula wrote:
> On Wed, 09 Nov 2022, Michal Wajdeczko <michal.wajdeczko at intel.com> 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.
>
>
>