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 2CDADC433FE for ; Fri, 11 Nov 2022 15:47:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B47610E196; Fri, 11 Nov 2022 15:47:46 +0000 (UTC) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id D3D8410E196 for ; Fri, 11 Nov 2022 15:47:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1668181662; x=1699717662; h=date:from:to:cc:subject:message-id:reply-to:references: mime-version:content-transfer-encoding:in-reply-to; bh=LXSdSOIoY5anSN8vXryPoGlfX/Q5Rk+LwVAzgKlPV18=; b=iH/FSblZfK1u0iZc1KkDKKyy4UwrZyolHiSICg0wL/VdMrl9+Vsqm+Yj gdK5FUmR0U1Pp5E9n8K+oBcF7JkGZxOBxi/pdGdZ7wbXyyNl+SRvEjNZv uzOWbYn4oqA3p553+LHl3B58fP0gjwgj7TjaEaYshxMCBsILTEUtPGjwZ 1ymNA1OYbSZZGdlTQnlX256Qm/gxT/hkfB9dWxb8gMYbRaIngj4ARzZdY Wdiu77r6Jg0BPAl9tQIMUMeEv3TWf51dDnaugtlhfcUDkQRzGFj2L/03f 7yXLjGLeTdJQx599bBYp1f78DJttliKywDyy3y3BK1lUbP35KjUoA2Q6J g==; X-IronPort-AV: E=McAfee;i="6500,9779,10528"; a="299133769" X-IronPort-AV: E=Sophos;i="5.96,156,1665471600"; d="scan'208";a="299133769" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2022 07:47:12 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10528"; a="588602362" X-IronPort-AV: E=Sophos;i="5.96,156,1665471600"; d="scan'208";a="588602362" Received: from ideak-desk.fi.intel.com ([10.237.68.144]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2022 07:47:10 -0800 Date: Fri, 11 Nov 2022 17:47:05 +0200 From: Imre Deak To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Message-ID: References: <20221107170917.3566758-2-imre.deak@intel.com> <20221108151828.3761358-1-imre.deak@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Intel-gfx] [PATCH v3 1/9] drm/i915: Allocate power domain set wakerefs dynamically X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: imre.deak@intel.com Cc: Jani Nikula , intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, Nov 11, 2022 at 03:52:08PM +0200, Ville Syrjälä wrote: > On Fri, Nov 11, 2022 at 03:43:54PM +0200, Ville Syrjälä wrote: > > On Fri, Nov 11, 2022 at 02:37:13PM +0200, Imre Deak wrote: > > > On Thu, Nov 10, 2022 at 11:49:19PM +0200, Ville Syrjälä wrote: > > > > On Thu, Nov 10, 2022 at 09:55:55PM +0200, Imre Deak wrote: > > > > > On Thu, Nov 10, 2022 at 09:11:20PM +0200, Ville Syrjälä wrote: > > > > > > On Tue, Nov 08, 2022 at 05:18:23PM +0200, Imre Deak wrote: > > > > > > > Since the intel_display_power_domain_set struct, currently its current > > > > > > > size close to 1kB, can be allocated on the stack, it's better to > > > > > > > allocate the per-domain wakeref pointer array - used for debugging - > > > > > > > within the struct dynamically, so do this. > > > > > > > > > > > > > > The memory freeing is guaranteed by the fact that the acquired domain > > > > > > > references tracked by the struct can't be leaked either. > > > > > > > > > > > > > > v2: > > > > > > > - Don't use fetch_and_zero() when freeing the wakerefs array. (Jani) > > > > > > > - Simplify intel_display_power_get/put_in_set(). (Jani) > > > > > > > - Check in intel_crtc_destroy() that the wakerefs array has been freed. > > > > > > > v3: > > > > > > > - Add intel_display_power_set_disabled() and a separate assert > > > > > > > function instead of open coding these. (Jani) > > > > > > > > > > > > > > Cc: Jani Nikula > > > > > > > Signed-off-by: Imre Deak > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_crtc.c | 11 ++ > > > > > > > .../drm/i915/display/intel_display_power.c | 109 ++++++++++++++---- > > > > > > > .../drm/i915/display/intel_display_power.h | 6 +- > > > > > > > 3 files changed, 104 insertions(+), 22 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c > > > > > > > index 037fc140b585c..c18d98bfe1a7c 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > > > > > > @@ -21,6 +21,7 @@ > > > > > > > #include "intel_crtc.h" > > > > > > > #include "intel_cursor.h" > > > > > > > #include "intel_display_debugfs.h" > > > > > > > +#include "intel_display_power.h" > > > > > > > #include "intel_display_trace.h" > > > > > > > #include "intel_display_types.h" > > > > > > > #include "intel_drrs.h" > > > > > > > @@ -37,6 +38,14 @@ static void assert_vblank_disabled(struct drm_crtc *crtc) > > > > > > > drm_crtc_vblank_put(crtc); > > > > > > > } > > > > > > > > > > > > > > +static void assert_power_domains_disabled(struct intel_crtc *crtc) > > > > > > > +{ > > > > > > > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > > > > > + > > > > > > > + drm_WARN_ON(&i915->drm, > > > > > > > + !intel_display_power_set_disabled(i915, &crtc->enabled_power_domains)); > > > > > > > +} > > > > > > > + > > > > > > > struct intel_crtc *intel_first_crtc(struct drm_i915_private *i915) > > > > > > > { > > > > > > > return to_intel_crtc(drm_crtc_from_index(&i915->drm, 0)); > > > > > > > @@ -204,6 +213,8 @@ static void intel_crtc_destroy(struct drm_crtc *_crtc) > > > > > > > > > > > > > > cpu_latency_qos_remove_request(&crtc->vblank_pm_qos); > > > > > > > > > > > > > > + assert_power_domains_disabled(crtc); > > > > > > > + > > > > > > > drm_crtc_cleanup(&crtc->base); > > > > > > > kfree(crtc); > > > > > > > } > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > > > > index 4c1de91e56ff9..ca63b4f1af41b 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > > > > @@ -830,20 +830,85 @@ void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv, > > > > > > > } > > > > > > > #endif > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > > > > > > > +static void > > > > > > > +add_domain_to_set(struct drm_i915_private *i915, > > > > > > > + struct intel_display_power_domain_set *power_domain_set, > > > > > > > + enum intel_display_power_domain domain, > > > > > > > + intel_wakeref_t wf) > > > > > > > +{ > > > > > > > + drm_WARN_ON(&i915->drm, test_bit(domain, power_domain_set->mask.bits)); > > > > > > > + > > > > > > > + if (!power_domain_set->wakerefs) > > > > > > > + power_domain_set->wakerefs = kcalloc(POWER_DOMAIN_NUM, > > > > > > > + sizeof(*power_domain_set->wakerefs), > > > > > > > + GFP_KERNEL); > > > > > > > + > > > > > > > + if (power_domain_set->wakerefs) > > > > > > > + power_domain_set->wakerefs[domain] = wf; > > > > > > > > > > > > So if the kcalloc() fails is it going to look like > > > > > > we're leaking power wakerefs? > > > > > > > > > > Yes, along with the alloc failure which is also logged. I assumed this > > > > > is enough to explain why wakeref tracking doesn't work afterwards, but I > > > > > suppose the wakeref could be untracked here in this case. > > > > > > > > I think a more clear message what is going on would be good. > > > > And probably preventing the spam from the wakerefs would > > > > also be good to make sure the whole thing doesn't get > > > > misdiagnosed as a real power ref leak. > > > > > > Ok, I can add a debug print about the failure and untrack the wakeref. > > > > The other idea that came to mind was to just preallocate this > > somehow. Looks like the only place where this even matters > > currently is hsw_get_pipe_config(). The other instance of > > this structure is already embedded in a kmalloced thing > > (struct intel_crtc). Can't think of a good way to allocate the whole struct, as the domain mask within it is still needed if the allocation failed. The API is simpler imo if the allocation happens internally, but yes not a big change if there is a good way to allocate only the array part. > Hmm. I wonder if this wakeref tracking is even really useful > in the readout path. We just do a put_all_in_set() in the end, > so can it even leak? If the function would incorrectly do an early return, the domains wouldn't be put. I think it makes sense to keep wakerefs working for this case as well. > -- > Ville Syrjälä > Intel