Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Mar 2023 11:03:57 -0400
From:   Rodrigo Vivi <rodrigo.vivi@intel.com>
To:     Andrzej Hajda <andrzej.hajda@intel.com>
CC:     Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        "Daniel Vetter" <daniel@ffwll.ch>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        "Chris Wilson" <chris.p.wilson@linux.intel.com>,
        Andi Shyti <andi.shyti@linux.intel.com>,
        Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH] drm/i915/gt: Hold a wakeref for the active VM
Message-ID: <ZCWk3Wb2eMpN9dw0@intel.com>
References: <20230330-hold_wakeref_for_active_vm-v1-1-baca712692f6@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230330-hold_wakeref_for_active_vm-v1-1-baca712692f6@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?C/HQ373cG9kdhr0q6KV9xf6HEdnJDq+WwXjiJGdK0jwb8crl2w7jP47lYoxY?=
 =?us-ascii?Q?It+xWRenGI+zF9H/MfCMsVsDoEkPIHbJ/U74tLZR9ShZ4qKS32cAZoz5DMxR?=
 =?us-ascii?Q?MdTcopBik36VxDQlYbZDV5A/uupiRi45slzKnbe0b8psGrRXdTfKaUrLiO+z?=
 =?us-ascii?Q?di3ydu2u1SSggKrC6lOriN0UrznuGfyJy5nx+h7Q43kbQCKqHkLo9Ob0aSQW?=
 =?us-ascii?Q?Hmys6fdB8PbXm008HcXUB6Q8tRQp51fuood4PJx+nVmsKe8ruyd4I9mv2w+1?=
 =?us-ascii?Q?ug1Ve6AD5/1BjZofJ6hMcVuOBM9SpAHzOvHr2dUd2m0M3z89ndzaRUQGuRZn?=
 =?us-ascii?Q?sRIqpyid4GZOhx5jPltzvmN01MkIuR9Vp6Ut43IEFcz9KK9Cc/LNK9+/Zd6m?=
 =?us-ascii?Q?6nVRzOUo9r/q7sNcEXdLnVF6B3DOzacCZuoZWA68OakBrvvNlEOH0d8ylyyi?=
 =?us-ascii?Q?84QeQy6PMInsh5Pjl/IL6Hg9sw+iJ42x+zPqOI3SWOrP+YZZEJ1OaiJasrkI?=
 =?us-ascii?Q?vZuyoRaasZrMkGX/CgIAwvQi5fg3wwv3wQUYx1ENahc3dyue48bsw2D8zNPv?=
 =?us-ascii?Q?3MIfK5MA2KUloTmuK0eB74bckOgxd4Efcx/k6gBus9UcvunXBoBOwM6y/uv6?=
 =?us-ascii?Q?5xhclPcXxC2oekuJY+KNkmYMrzfG47t/FkzYUy8EktZj2g66I4nxDII+1Qp8?=
 =?us-ascii?Q?fmG37FdYiuNCdTK6oDeSwRuN8b4xxTdDT1O+KiqDcLkoVSNgDgys+EIpCGBL?=
 =?us-ascii?Q?Krcft9Cu0O2qHpJl5JByUhUxojSF4sx6XNsYduTV4A9t7CKPYMG/cG+OKlK9?=
 =?us-ascii?Q?hiNfpVrtO6thh8WTqCV/ARjYHVviftEuqN4/fJgf3l4ke5EE+BxrFF12eoru?=
 =?us-ascii?Q?4unM9UaVos9V6np0st+VdO9on89RdabCUFvr6t9YMHVTadL3K+SNrttdP027?=
 =?us-ascii?Q?qJI+xXvBNudA4loWsPT1dpIjkiuMgWe56L4kYWJtr478rAPDGuNgHS5otjfk?=
 =?us-ascii?Q?nm2d1CC/wdvf1//8RttWnFHWl5APrn4mJm3tDYdyVco7+e36oWywUaWh1s8b?=
 =?us-ascii?Q?NuEsFDFoqa3ep7LqZTWc3jUw5eVng7gPZgopBWhaQ1lveSdNIKz1rToJy7Jy?=
 =?us-ascii?Q?gfmzw4adcAeXmcQVYQPi9iUM/wrCGISr2U1dRvSGdUcu6p2wAyTkEFFSsEiR?=
 =?us-ascii?Q?dwpwwm8W/YlGJB6Kp4zErbHHidCg2Yiz6k0IZeGrcElWkmKk7I4FrOBr+OMS?=
 =?us-ascii?Q?bdt4BxDFzjMbz+JYce3dgkdYozXFSTsmgXk1bTe/ulrQohayc1BKzpwZ3d22?=
 =?us-ascii?Q?1w4oMmDFfCqv+3zwsB4VvIDfaZr5ktMikhW4VrvA3cibM/q0vYotUUNtwMYb?=
 =?us-ascii?Q?rzLXRPeISjxwFYS5gtiF25y8LFYgG38D25nTUH1QMIIX11lSQfI0BtTEcaEv?=
 =?us-ascii?Q?2vqmiqymfEoiM+wzJuMyo0As7a7+tej5tsRjL85Sh5FM/xspo1UdhpZGm7sT?=
 =?us-ascii?Q?PMohmhkek+jMmPHb4zdIq0XGyj+90yeR938nlwb7Wblv5eFJaYUmVVhIIwKE?=
 =?us-ascii?Q?35DoAp5p+oQ2/CeT2TaD0EOBbKp1OZH2rnv7AbEb?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a7f5a44-da30-4f7a-83a5-08db31300064
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6059.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 15:04:04.1740
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rq0uc9id8Jl5Qss+y7O78hDQL+uEGODvffAvxZBAFDP7iG32trv8RUZNRcy0yeKzccEu/uzVlBODniUJetfKkg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4946
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE,
        SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 30, 2023 at 04:35:39PM +0200, Andrzej Hajda wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> There may be a disconnect between the GT used by the engine and the GT
> used for the VM, requiring us to hold a wakeref on both while the GPU is
> active with this request.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> [ahajda: removed not-yet-upstremed wakeref tracking bits]
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_context.h   | 15 +++++++++++----
>  drivers/gpu/drm/i915/gt/intel_engine_pm.c |  3 +++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 0a8d553da3f439..48f888c3da083b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -14,6 +14,7 @@
>  #include "i915_drv.h"
>  #include "intel_context_types.h"
>  #include "intel_engine_types.h"
> +#include "intel_gt_pm.h"
>  #include "intel_ring_types.h"
>  #include "intel_timeline_types.h"
>  #include "i915_trace.h"
> @@ -207,8 +208,11 @@ void intel_context_exit_engine(struct intel_context *ce);
>  static inline void intel_context_enter(struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->timeline->mutex);
> -	if (!ce->active_count++)
> -		ce->ops->enter(ce);
> +	if (ce->active_count++)
> +		return;
> +
> +	ce->ops->enter(ce);
> +	intel_gt_pm_get(ce->vm->gt);
>  }
>  
>  static inline void intel_context_mark_active(struct intel_context *ce)
> @@ -222,8 +226,11 @@ static inline void intel_context_exit(struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->timeline->mutex);
>  	GEM_BUG_ON(!ce->active_count);
> -	if (!--ce->active_count)
> -		ce->ops->exit(ce);
> +	if (--ce->active_count)
> +		return;
> +
> +	intel_gt_pm_put_async(ce->vm->gt);
> +	ce->ops->exit(ce);

so far so good. this all matches the commit msg and seems a good move.


>  }
>  
>  static inline struct intel_context *intel_context_get(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index e971b153fda976..ac0566c5e99e17 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -114,6 +114,9 @@ __queue_and_release_pm(struct i915_request *rq,
>  
>  	ENGINE_TRACE(engine, "parking\n");
>  
> +	GEM_BUG_ON(rq->context->active_count != 1);

why do you need this here? should it be a separated patch with
separated explanation?


> +	__intel_gt_pm_get(engine->gt);

why? I mean, why the get in the release pm?
and where's the put for this get?
should it be a separated patch as well?

> +
>  	/*
>  	 * We have to serialise all potential retirement paths with our
>  	 * submission, as we don't want to underflow either the
> 
> ---
> base-commit: 3385d6482cd60f2a0bbb0fa97b70ae7dbba4f95c
> change-id: 20230330-hold_wakeref_for_active_vm-7f013a449ef3
> 
> Best regards,
> -- 
> Andrzej Hajda <andrzej.hajda@intel.com>