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 BF262C25B74 for ; Thu, 30 May 2024 07:50:13 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 60F7910E60E; Thu, 30 May 2024 07:50:11 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="G3ZVGry2"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2EB6C10E25E for ; Thu, 30 May 2024 07:50:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1717055401; x=1748591401; h=from:to:subject:in-reply-to:references:date:message-id: mime-version:content-transfer-encoding; bh=53JSzNw0/trbJsj2t6EuzttuiGcXw14M0CMb3R0baHo=; b=G3ZVGry20ZJww8qt2f0sGrrj6nLYJK235aQeY0VFu4Jq4kI9BEHlFNP0 WSK1YZCGwgHSi7Gg1/raQb9R7r08J2Hg1O5e/ZVzH9FEks0lEFelWZsMw u8fa9t/B/OruNtioYQzH7nUq59tHous1AgnR7kXjmA5apABGfyd6Fojz8 TW+aroI+w9JHCSJxP8k6mhfHnHKRrdOaIl/gFRF+NuCxNxuZclwZFpqpT OMjMPcKIdg0Yq9qUIY8g/pIuJzKjsV+lQiTTvI51ptMXBsFd2d66OetGA s4lJOmal/8sFDiyxwsXnWmCzmkbENQ0C9BAav17kQgQLt4q59snvv+MTo A==; X-CSE-ConnectionGUID: BjS1CJgMRf+hc8xkJmkgug== X-CSE-MsgGUID: wdU5s22mQcuPRmH8+y7JPg== X-IronPort-AV: E=McAfee;i="6600,9927,11087"; a="13362225" X-IronPort-AV: E=Sophos;i="6.08,199,1712646000"; d="scan'208";a="13362225" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by orvoesa112.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 May 2024 00:50:00 -0700 X-CSE-ConnectionGUID: 2nriOCFgTh+b+5HGlmqJFw== X-CSE-MsgGUID: rqqm3XuCSjisRSgfp2WT4w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,199,1712646000"; d="scan'208";a="58898307" Received: from fdefranc-mobl3.ger.corp.intel.com (HELO localhost) ([10.245.246.132]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 May 2024 00:49:59 -0700 From: Jani Nikula To: John Harrison , Michal Wajdeczko , dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2] drm/print: Introduce drm_line_printer In-Reply-To: <1bf31a4b-fede-4044-8390-abb2b833608d@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20240528130622.1152-1-michal.wajdeczko@intel.com> <1bf31a4b-fede-4044-8390-abb2b833608d@intel.com> Date: Thu, 30 May 2024 10:49:56 +0300 Message-ID: <877cfbivrv.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 Wed, 29 May 2024, John Harrison wrote: > On 5/28/2024 06:06, Michal Wajdeczko wrote: >> 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 >> Cc: Jani Nikula >> Cc: John Harrison >> --- >> v2: don't abuse prefix, use union instead (Jani) >> don't use 'dp' as name, prefer 'p' (Jani) >> add support for unique series identifier (John) >> --- >> drivers/gpu/drm/drm_print.c | 14 ++++++++ >> include/drm/drm_print.h | 68 ++++++++++++++++++++++++++++++++++++- >> 2 files changed, 81 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c >> index cf2efb44722c..be9cbebff5b3 100644 >> --- a/drivers/gpu/drm/drm_print.c >> +++ b/drivers/gpu/drm/drm_print.c >> @@ -214,6 +214,20 @@ void __drm_printfn_err(struct drm_printer *p, struc= t va_format *vaf) >> } >> EXPORT_SYMBOL(__drm_printfn_err); >>=20=20=20 >> +void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf) >> +{ >> + unsigned int counter =3D ++p->line.counter; > Wrong units, but see below anyway... > >> + const char *prefix =3D p->prefix ?: ""; >> + const char *pad =3D p->prefix ? " " : ""; >> + >> + if (p->line.series) >> + drm_printf(p->arg, "%s%s%u.%u: %pV", >> + prefix, pad, p->line.series, counter, vaf); >> + else >> + drm_printf(p->arg, "%s%s%u: %pV", prefix, pad, counter, vaf); >> +} >> +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..f4d9b98d7909 100644 >> --- a/include/drm/drm_print.h >> +++ b/include/drm/drm_print.h >> @@ -176,7 +176,13 @@ struct drm_printer { >> void (*puts)(struct drm_printer *p, const char *str); >> void *arg; >> const char *prefix; >> - enum drm_debug_category category; >> + union { >> + enum drm_debug_category category; >> + struct { >> + unsigned short series; >> + unsigned short counter; > Any particular reason for using 'short' rather than 'int'? Short is only= =20 > 16bits right? That might seem huge but a GuC log buffer with 16MB debug=20 > log (and minimum sizes for the other sections) when dumped via the=20 > original debugfs hexdump mechanism is 1,245,444 lines. That line count=20 > goes down a lot when you start using longer lines for the dump, but it=20 > is still in the tens of thousands of lines.=C2=A0 So limiting to 16 bits = is=20 > an overflow just waiting to happen. > >> + } line; >> + }; >> }; >>=20=20=20 >> void __drm_printfn_coredump(struct drm_printer *p, struct va_format *v= af); >> @@ -186,6 +192,7 @@ void __drm_puts_seq_file(struct drm_printer *p, cons= t 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); >>=20=20=20 >> __printf(2, 3) >> void drm_printf(struct drm_printer *p, const char *f, ...); >> @@ -357,6 +364,65 @@ static inline struct drm_printer drm_err_printer(st= ruct drm_device *drm, >> return p; >> } >>=20=20=20 >> +/** >> + * drm_line_printer - construct a &drm_printer that prefixes outputs wi= th line numbers >> + * @p: the &struct drm_printer which actually generates the output >> + * @prefix: optional output prefix, or NULL for no prefix >> + * @series: optional unique series identifier, or 0 to omit identifier = in 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. He= lpful >> + * while capturing some crash data. >> + * >> + * Example 1:: >> + * >> + * void crash_dump(struct drm_device *drm) >> + * { >> + * static unsigned short id; >> + * struct drm_printer p =3D drm_err_printer(drm, "crash"); >> + * struct drm_printer lp =3D drm_line_printer(&p, "dump", ++id); > Is there any benefit or other reason to split the prefix across two=20 > separate printers? It seems like a source of confusion. As in, the code=20 > will allow a double prefix, there is not much you can do about that=20 > because losing the prefix from drm_line_printer would mean no prefix at=20 > all when not using drm_err_printer underneath. But why explicitly split=20 > the message across both printers in the usage example? This is saying=20 > that this is the recommended way to use the interface, but with no=20 > explanation of why the split is required or how the split should be done. You could have a printer, and then add two separate line counted blocks. struct drm_printer p =3D drm_err_printer(drm, "parent"); struct drm_printer lp1 =3D drm_line_printer(&p, "child 1", 0); ... struct drm_printer lp2 =3D drm_line_printer(&p, "child 2", 0); ... p could be defined elsewhere and passed into separate functions which each have the line printing. The two prefixes can be useful. > Also, there is really no specific connection to crashes. The purpose of=20 > this is for allowing the dumping of multi-line blocks of data. One use=20 > is for debugging crashes. But there are many debug operations that=20 > require the same dump and do not involve a crash. And this support is=20 > certainly not intended to be used on general end-user GPU hangs. For=20 > those, everything should be part of the devcoredump core file that is=20 > produced and accessible via sysfs. We absolutely should not be dumping=20 > huge data blocks in customer release drivers except in very extreme=20 > circumstances. A devcoredump implementation could use a drm_printer too? Is this only about bikeshedding the example? I'm not sure I get the point here. > >> + * >> + * drm_printf(&lp, "foo"); >> + * drm_printf(&lp, "bar"); >> + * } >> + * >> + * Above code will print into the dmesg something like:: >> + * >> + * [ ] 0000:00:00.0: [drm] *ERROR* crash dump 1.1: foo >> + * [ ] 0000:00:00.0: [drm] *ERROR* crash dump 1.2: bar >> + * >> + * Example 2:: >> + * >> + * void line_dump(struct device *dev) >> + * { >> + * struct drm_printer p =3D drm_info_printer(dev); >> + * struct drm_printer lp =3D drm_line_printer(&p, NULL, 0); >> + * >> + * drm_printf(&lp, "foo"); >> + * drm_printf(&lp, "bar"); >> + * } >> + * >> + * Above code will print:: >> + * >> + * [ ] 0000:00:00.0: [drm] 1: foo >> + * [ ] 0000:00:00.0: [drm] 2: bar > Not really seeing the point of having two examples listed. The first=20 > includes all the optional extras, the second is just repeating with no=20 > new information. You see how the "series" param behaves? BR, Jani. > > John. > >> + * >> + * RETURNS: >> + * The &drm_printer object >> + */ >> +static inline struct drm_printer drm_line_printer(struct drm_printer *p, >> + const char *prefix, >> + unsigned short series) >> +{ >> + struct drm_printer lp =3D { >> + .printfn =3D __drm_printfn_line, >> + .arg =3D p, >> + .prefix =3D prefix, >> + .line =3D { .series =3D series, }, >> + }; >> + return lp; >> +} >> + >> /* >> * struct device based logging >> * > --=20 Jani Nikula, Intel