From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9F654C25B74 for ; Fri, 24 May 2024 08:00:27 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 13E6310E573; Fri, 24 May 2024 08:00:10 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="ApgLr+eB"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.19]) by gabe.freedesktop.org (Postfix) with ESMTPS id 00BAB10E0EE for ; Fri, 24 May 2024 08:00:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1716537604; x=1748073604; h=from:to:subject:in-reply-to:references:date:message-id: mime-version; bh=fOK/TVHjBWTbttyQS/h2R1BBdxC0KJFN0T3/7figorQ=; b=ApgLr+eBV5jSd4JrW2N1Bpju0Q+8btFOQd3Dajphb0QGgly0ZgssNlbm aNiQn/l/wSMZfJPHO4BFH2p7TUidD682g/J5FOgA9GrivWrNed1kfzFGx oL/FvJ3Qycd2eECux7NEjJbf36oBEOdyFEQ8BKq8JXtMlTdh5bHKqNjbw IB/QRCft/6pGIFDhZ0CjsaJ1Er+Pim3phKUilSSCxh8Z0K4oT5iqguiDl G8Ip4GDOKCR3WIli7BBRWkIlnmBxq5mWQGmnIPJj5PTb7z8j1sBMcrOLE pCtvdGHwi2EKw0JnAX4sq1ElooxZc98xqiAYqZS90YcqSw0lFxKujiRvU A==; X-CSE-ConnectionGUID: rYmrDKEAQhWLoHxulsn4pw== X-CSE-MsgGUID: 5u8pfjb3TbuCFXGVKodx6g== X-IronPort-AV: E=McAfee;i="6600,9927,11081"; a="12753375" X-IronPort-AV: E=Sophos;i="6.08,184,1712646000"; d="scan'208";a="12753375" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by orvoesa111.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 May 2024 01:00:02 -0700 X-CSE-ConnectionGUID: T52tOStORx+4KCVN5FknKQ== X-CSE-MsgGUID: 2V2XwxtpTfCDSN7ePRBIRQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,184,1712646000"; d="scan'208";a="38927485" Received: from bergbenj-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.246.108]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 May 2024 01:00:00 -0700 From: Jani Nikula To: John Harrison , dri-devel , Michal Wajdeczko Subject: Re: [RFC] drm/print: Introduce drm_line_printer In-Reply-To: <6d88ab01-f2c8-4791-9802-87151b178c0b@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20240514145631.2128-1-michal.wajdeczko@intel.com> <6d88ab01-f2c8-4791-9802-87151b178c0b@intel.com> Date: Fri, 24 May 2024 10:59:56 +0300 Message-ID: <87zfsfod1f.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, 23 May 2024, John Harrison wrote: > On 5/23/2024 16:54, Daniele Ceraolo Spurio wrote: >> -------- Forwarded Message -------- >> Subject: [RFC] drm/print: Introduce drm_line_printer >> Date: Tue, 14 May 2024 16:56:31 +0200 >> From: Michal Wajdeczko >> To: dri-devel@lists.freedesktop.org >> >> >> >> This drm printer wrapper can be used to increase the robustness of >> the captured output generated by any other drm_printer to make sure >> we didn't lost any intermediate lines of the output by adding line >> numbers to each output line. Helpful for capturing some crash data. >> >> Signed-off-by: Michal Wajdeczko >> --- >> drivers/gpu/drm/drm_print.c | 9 +++++++++ >> include/drm/drm_print.h | 37 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c >> index cf2efb44722c..d6fb50d3407a 100644 >> --- a/drivers/gpu/drm/drm_print.c >> +++ b/drivers/gpu/drm/drm_print.c >> @@ -214,6 +214,15 @@ void __drm_printfn_err(struct drm_printer *p, >> struct va_format *vaf) >> } >> EXPORT_SYMBOL(__drm_printfn_err); >> +void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf) >> +{ >> + unsigned int line = (uintptr_t)(++p->prefix); > The prefix field is officially supposed to be a const char *. There is > no documentation to say that this is intended to be used as a private > data field by random printer wrappers. So overloading it like this feels > very hacky and dangerous. Also, you are mixing types - uintptr_t then > uint. So an arch with 64-bit pointers but only 32-bit ints would hit a > truncated compiler warning? I already commented on abusing the type in another reply. I think making prefix a union would dodge that issue. Otherwise, I think each printer can pretty much use the arg and prefix members as they see fit. It's not mucking the printer being wrapped. > >> + struct drm_printer *dp = p->arg; >> + >> + drm_printf(dp, "%u: %pV", line, vaf); > This is insufficient. As previously commented, there needs to be a > global counter as well as a local line counter. The global count must be > global to at least whatever entity is generating a specific set of > prints. Being global to a higher level, e.g. kernel global, is fine. But > without that, two concurrent dumps that get interleaved can be > impossible to separate resulting in a useless bug report/log. > > The prefix field could potentially be split into a 16:16 global:local > index with the global master just being a static u16 inside that > function. With the first print call to a given drm_printer object being > defined by the global value being zero. And it then sets the global > value to the next increment skipping over zero on a 16-bit wrap around. > But see above about prefix not being intended for such purposes. So now > you are just piling hacks upon hacks. To tackle that issue, I think I'd add a "unique id" kind of parameter to drm_line_printer(). If the user needs more uniqueness, they can maintain it locally (in a data structure or static or whatever) and id++ it. Could even skip it in printing if set to 0 if it's not likely the caller needs it. This dodges the issue of having to store global stuff in the printer, and keeps output lean when not needed. For example: static int id; struct drm_printer dp = drm_err_printer(drm, "crash"); struct drm_printer lp = drm_line_printer(&dp, ++id); > Plus it would be much nicer output to have the ability to put an > arbitrary prefix in front of the G.L number, as per the original > implementation. The whole point of this is to aid identification of > otherwise uniform data such as hexdumps. So anything that makes it less > clear is bad. The prefix in the printer being wrapped is intact, so you could add it there. In the above example, it's "crash". BR, Jani. > > John. > > >> +} >> +EXPORT_SYMBOL(__drm_printfn_line); >> + >> /** >> * drm_puts - print a const string to a &drm_printer stream >> * @p: the &drm printer >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h >> index 089950ad8681..58cc73c53853 100644 >> --- a/include/drm/drm_print.h >> +++ b/include/drm/drm_print.h >> @@ -186,6 +186,7 @@ void __drm_puts_seq_file(struct drm_printer *p, >> const char *str); >> void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf); >> void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf); >> void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf); >> +void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf); >> __printf(2, 3) >> void drm_printf(struct drm_printer *p, const char *f, ...); >> @@ -357,6 +358,42 @@ static inline struct drm_printer >> drm_err_printer(struct drm_device *drm, >> return p; >> } >> +/** >> + * drm_line_printer - construct a &drm_printer that prefixes outputs >> with line numbers >> + * @dp: the &struct drm_printer which actually generates the output >> + * >> + * This printer can be used to increase the robustness of the >> captured output >> + * to make sure we didn't lost any intermediate lines of the output. >> Helpful >> + * while capturing some crash data. >> + * >> + * For example:: >> + * >> + * void crash_dump(struct drm_device *drm) >> + * { >> + * struct drm_printer dp = drm_err_printer(drm, "crash"); >> + * struct drm_printer lp = drm_line_printer(&dp); >> + * >> + * drm_printf(&lp, "foo"); >> + * drm_printf(&lp, "bar"); >> + * } >> + * >> + * Above code will print into the dmesg something like:: >> + * >> + * [ ] 0000:00:00.0: [drm] *ERROR* crash 1: foo >> + * [ ] 0000:00:00.0: [drm] *ERROR* crash 2: bar >> + * >> + * RETURNS: >> + * The &drm_printer object >> + */ >> +static inline struct drm_printer drm_line_printer(struct drm_printer *dp) >> +{ >> + struct drm_printer lp = { >> + .printfn = __drm_printfn_line, >> + .arg = dp, >> + }; >> + return lp; >> +} >> + >> /* >> * struct device based logging >> * >> -- >> 2.43.0 >> -- Jani Nikula, Intel