Re: [Intel-gfx] [PATCH v1 02/11] drm: drop uapi dependency from drm_print.h

From: Jani Nikula <jani.nikula@intel.com>
To: Sam Ravnborg <sam@ravnborg.org>, "Koenig\,
	Christian" <Christian.Koenig@amd.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	David Airlie <airlied@linux.ie>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Sean Paul <seanpaul@chromium.org>,
	Thierry Reding <treding@nvidia.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [Intel-gfx] [PATCH v1 02/11] drm: drop uapi dependency from drm_print.h
Date: Fri, 02 Aug 2019 16:48:19 +0300	[thread overview]
Message-ID: <87o917lluk.fsf@intel.com> (raw)
In-Reply-To: <20190729175011.GA1753@ravnborg.org>

On Mon, 29 Jul 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Christian.
>
> On Mon, Jul 29, 2019 at 03:28:15PM +0000, Koenig, Christian wrote:
>> Am 29.07.19 um 16:35 schrieb Sam Ravnborg:
>> >>>> Even then it so useless (which drm driver is this message for???) that I
>> >>>> want to remove them all :(
>> >>> Yeah, agree. I mean it is nice if the core drm functions use a prefix
>> >>> for debug output.
>> >>>
>> >>> But I actually don't see the point for individual drivers.
>> >> We should all migrate to the versions with device...
>> > Just to do an xkdc.com/927 I have considered:
>> >
>> > drm_err(const struct drm_device *drm, ...)
>> > drm_info(const struct drm_device *drm, ...)
>> >
>> > drm_kms_err(const struct drm_device *drm, ...)
>> > drm_kms_info(const struct drm_device *drm, ...))
>> 
>> Why not get completely rid of those and just use dev_err, dev_warn, 
>> pr_err, pr_warn etc?
>> 
>> I mean is it useful to have this extra printing subsystem in DRM while 
>> the standard Linux one actually does a better job?
> The added functionality of drm_xxx_err would be to keep the current
> drm.debug=0x1f filtering on the command-line.
> I do not think we can do this with the standard logging.

Correct. I'd love the benefits of dynamic debug, but there's no support
for the kind of message categories that we do with drm.debug.

I've contemplated switching i915 over to using the variants where you
pass the device, but I really really don't like the idea of:

- 	DRM_DEBUG_KMS("hello\n");
+ 	DRM_DEV_DEBUG_KMS(i915->drm.dev, "hello\n");

Where i915 is our private wrapper for drm_device. I think it's just too
much ugly uppercase boilerplate, and a large portion of the debugs would
have to span at least an extra line after that.

I'd also very much prefer passing just struct *drm_device instead of
struct *device. In that, I think the needs of the few have prevailed
over the needs of the many. I'd definitely prefer drm_err(const struct
drm_device *drm, ...) and friends over the current ones.

Frankly, I've actually ended up thinking about adding our own, short
i915 wrappers for our needs. :(

BR,
Jani.

>
> And then we can prefix every logging with driver name and device name.
>
> The idea is to make a thin layer on top of the existing pr_xxx() functions.
> So not a full subsystem, only a wrapper on top of what we already have.
>
> Anyway, idle talk only. We need patches and sample output if we should
> discuss more.
>
> 	Sam
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel