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 63D1AC25B75 for ; Wed, 15 May 2024 13:18:32 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9BFA410E723; Wed, 15 May 2024 13:18:31 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="UCjCCKsI"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) by gabe.freedesktop.org (Postfix) with ESMTPS id 75B0410E723 for ; Wed, 15 May 2024 13:18:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1715779111; x=1747315111; h=from:to:subject:in-reply-to:references:date:message-id: mime-version; bh=FYEVEehj3lrSoKVtdfSb4gyx8lZAMNG0IWsTUQ1reAc=; b=UCjCCKsIKQOc4WKaeLZh/X6yf9EO1gGU5c4LN06nzahGpPCLqNm2yuUP xEyWA1aFP8AdMnotTpUYhTIfU1NFTC0YbnW7H1YYjce0JLzOmLmx+Uifl V4psvZksCTytY7vJReShyTQSkZXQ9OF8lQc9AOQzwytFXGJBXVeXZQdwu 0MRG/3Bn4i5zihg49q/zg3GtugmaZAq9pQiXZYxn26lRmIiuks5ezzPF5 aE0OGDpDwYb1yROOv73APdpc/sT9czIcMpNNwDdsccUZXVsLiP4/lzviB QDb0bVUK8irw+/YSqXDAwD22DZh7zdgc1OCIRLBSA8+WUPKdW28OKjdm9 g==; X-CSE-ConnectionGUID: naRSTl1kShWtnwNAg66cRA== X-CSE-MsgGUID: vWTL989/Rtysy8/369BHHA== X-IronPort-AV: E=McAfee;i="6600,9927,11073"; a="29318128" X-IronPort-AV: E=Sophos;i="6.08,161,1712646000"; d="scan'208";a="29318128" Received: from fmviesa005.fm.intel.com ([10.60.135.145]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2024 06:18:30 -0700 X-CSE-ConnectionGUID: hNJhsufDQOqdmXk7IGHuyg== X-CSE-MsgGUID: zjSRxRelQ/qAvoxboq9FkQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,161,1712646000"; d="scan'208";a="35520327" Received: from mwiniars-desk2.ger.corp.intel.com (HELO localhost) ([10.245.246.141]) by fmviesa005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2024 06:18:28 -0700 From: Jani Nikula To: Michal Wajdeczko , dri-devel@lists.freedesktop.org Subject: Re: [RFC] drm/print: Introduce drm_line_printer In-Reply-To: <08831b00-7af1-4f06-b0fd-4ed4179fa528@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20240514145631.2128-1-michal.wajdeczko@intel.com> <87seyjcsay.fsf@intel.com> <08831b00-7af1-4f06-b0fd-4ed4179fa528@intel.com> Date: Wed, 15 May 2024 16:18:25 +0300 Message-ID: <878r0bcixq.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 Wed, 15 May 2024, Michal Wajdeczko wrote: > On 15.05.2024 11:56, Jani Nikula wrote: >> On Tue, 14 May 2024, 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. >> >> Interesting. Nothing another level of abstraction can't solve. ;) >> >> Except maybe it'll make adding function names to debug printers harder. > > but why? primary printer should work in the same way with or without > this line printer Because of __builtin_return_address(0). But currently __drm_printfn_dbg() doesn't use that anyway, because it would already be off... Not really a meaningful argument against this patch, to be fair. >> >> No strong feelings either way about it, I'll let others chime in. > > forgot to mention that this new printer was aimed to simplify the manual > and error prone work done as part of [1] but I'm afraid there is little > traction to have any kind of generic solution at all, since it is > considered as 'over engineering', even at the cost of trashing other > printers that don't need extra robustness > > [1] https://patchwork.freedesktop.org/patch/593223/?series=133349&rev=1 > >> >> A few comments inline. >> >> >> BR, >> Jani. >> >>> >>> 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); >> >> Subtle. Might warrant adding a union in struct drm_printer for clarity. > > good idea > >> >>> + struct drm_printer *dp = p->arg; >>> + >>> + drm_printf(dp, "%u: %pV", line, 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..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) >> >> Just p is customary for the drm_printer. "dp" gives me too much Display >> Port vibes. > > ha, it was 'p' initially, but then it was still asymmetric compared to > __drm_printfn_line where 'p' was actually referring to the line printer > > maybe best option would be to don't have local vars at all: > > drm_printf((struct drm_printer *)p->arg, "%u: %pV", p->line, vaf); The cast is actually not required. BR, Jani. > >> >>> +{ >>> + struct drm_printer lp = { >>> + .printfn = __drm_printfn_line, >>> + .arg = dp, >>> + }; >>> + return lp; >>> +} >>> + >>> /* >>> * struct device based logging >>> * >> -- Jani Nikula, Intel