On 25/01/2018 11:32, Jani Nikula wrote:
On Wed, 24 Jan 2018, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 24/01/2018 16:23, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-01-24 16:18:15)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
This series tries to solve a few issues in the current DRM logging code to
primarily make it clearer which messages belong to which driver.
Main problem is that currently some logging functions allow individual drivers
to override the log prefix (since they are defined as macros, or static
inlines), while other hardcode the "drm" prefix into them due being situated in
the DRM core modules.
Another thing is that I noticed the DRM_NAME macro which is used for this is
defined in the uAPI header and had a comment which looked outdated.
Therefore I introduce a new define, called, DRM_LOG_NAME, this time defined
internally in the kernel headers and not exported in the uAPI.
I also refactored some logging functions to take this string as a parameter
instead of hardcoding it.
Individual drivers can then override this define to make DRM logging functions
prefix their message with the respective driver prefix.
End result in the case of the i915 driver looks like this:
Old log:
[drm] Found 128MB of eDRAM
[drm:skl_enable_dc6 [i915]] Enabling DC6
New log:
[i915] Found 128MB of eDRAM
[i915:skl_enable_dc6 [i915]] Enabling DC6
And still not conforming to the standard logging string. DRM_LOG should
What is the standard logging string? the dev_ one?
be killed off as an anachronistic OS compat layer.
You mean only dev variants should be kept?
I think the DRM_LOG_NAME override mechanism is too fragile, as it
depends on #include ordering. For our driver, I think it basically means
always including one of our headers (i915_drv.h) first everywhere (to
have a single point of truth for DRM_LOG_NAME), and including
drm_print.h first from there. That's not currently true, and I don't
want to see a massive #include reordering patchset to make it so.
This is like pr_fmt() which I think has been a mistake and should not be
repeated.
I think the direction to go is using dev_printk, dev_dbg, dev_err,
dev_warn, and friends, which use dev_driver_string internally. We
already have some drm wrappers for those. The problem with them is
passing dev, and I think that's the problem we should think about.
We also seem to have opted to use drm_dev_printk (which calls dev_printk
or printk) for DRM_DEV_DEBUG and friends. This is arguably a bad choice,
because using dev_dbg would let us make use of dynamic debug.
I agree the current state is messy and that a better improvement could
be made. And that the macro string approach is ugly.
To my defence, it sounded like a smaller amount of work to make it at
least a little bit better. The way I implemented it, as long as the
define is before any include directives it will work as expected. And on
my T460p which has both nouveau and i915 it makes the kernel log a bit
less confusing. But yeah, it is a marginal user base.
Scratch this then, previous posting was only two years ago so it can
wait some more for a more thorough etc rework.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
