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. BR, Jani. > > Regards, > > Tvrtko > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
