From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 100C821577A for ; Fri, 20 Dec 2024 14:11:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734703901; cv=none; b=HMnM37MyhvRQ2D1IMZODX1gK1H6Dca1Sn2kTQ6hmcs68JZ8zMDT+628/3QLOTzRPsFzoK1rkN9rZ/ELpdTUGQ6k6J61k8aESZ2GGRBWdPsCDQwkV6NitTcJ+MmrovXvcmOX+4zov3OvhgSgSdsOarkJrXR+EiKxzodygz1kFqGU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734703901; c=relaxed/simple; bh=tXG9+6xIhGDGxa6ol7ehNCy/Egn12Mcieruz73qr/a8=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=XMJSeFj3V8AKt/E+V3Th5SQbEWIPhN1M2PgDuygEz5dh4takCdU95vq0WIvAXdl1lmj1aO7Z3q9REq1ZXgLlNzbO2YMLh6a8fT58O8Yq6MVAraNCpQNL61XX/xtSu03RvzhATTB9XYOKYZksey1tULIeh323G0At4Io61YXvjfo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=JkTZ06r1; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="JkTZ06r1" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1734703898; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KR/Ybg3lzFNNrynj6GP2dfr+h48F8Y8mlhL1ZrgmhI0=; b=JkTZ06r1ZECNnT3QzmRt0z7JGoVU/OHdNRK0uyCf4RFxVfqKPwFvTtVJL505sZbm/S3Crp BbsqyLv/Ry0rlT1/aLt37vhKzsiwic7iTquFamND7swmd9W613pdK0f8V4gIQravuZglxu LUm4GOAi45FxYWy17YHvoTunLJBamz8= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-685-OBZjmfrONamD-6qYh4gWCg-1; Fri, 20 Dec 2024 09:11:37 -0500 X-MC-Unique: OBZjmfrONamD-6qYh4gWCg-1 X-Mimecast-MFC-AGG-ID: OBZjmfrONamD-6qYh4gWCg Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-43619b135bcso11063955e9.1 for ; Fri, 20 Dec 2024 06:11:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734703896; x=1735308696; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=KR/Ybg3lzFNNrynj6GP2dfr+h48F8Y8mlhL1ZrgmhI0=; b=kZkpJzhVTZpiH+m+NB2HxMWcK8IOsbqRtdjSBAwRRKZlnSzi7NcxBa+QmBASnHfA8E n6kiBpeLlS0OpoxILO7B6DvNFFXYSJmA12OJ8jFOods7Dsff0uxamIjpAp8r2sZy5M6r I4+qWL8As1PZBQbkYYJWfOKiQs9d598QcVupVcTQjwAtssd/lDoAngNEp3pm6UOPBI1Z hS2Rfwicyycm6n6t/OJo7WHWMKGfmI5lTfbFTpeCbK8EYHUKo+fKn/OYLTMxSuMA6m7b jr9zsoTM3ZWJv4xwqfPQfYOP8qqpNdSAaN2KOBPU5VkffQ3gHL2GYZjMesflEAD46x8N 6X6A== X-Forwarded-Encrypted: i=1; AJvYcCVto0719yPnW1NGfpkGohk9VbXLmTMRzlCcmPtJesH/Xb4yCvHrbBce906I9dXVa2JtwlePnpjZDz1Gnck=@vger.kernel.org X-Gm-Message-State: AOJu0YzFyHMe7SUFTq8GIMR8lFk4/J43iCgw7T0nESbTirWjP8p/qWRA EUB3rEr58fZXR4+m4SHkg+ARYfjtD+5djxYGsgai/SmJq/cnsQe971di6VW3ts46O8w1M5whNCy ghko23fWVdgWe/4PijpwwpII4gU49N5I0Ynu3xb4wCq0Y/tBLzSCjyXpwhlJ+NA== X-Gm-Gg: ASbGncv7+AkkzXngG+9rrv7+tW914k74N0ojQUVcBcWQE/T86wK4EeSNHSV4g3OneZp zYu8itxfF/bmt9uSQZT4bZXRWZU8W4/cl0ZNqv7sRP5VkaZvNCmamsBYiD5PcAnT2Qj5BCuQWae hm8G5rFYVht3JYVxL+RXLNEi3s7miin+cicbxK53UOGDikn/rl9UPMnVaoNnBIdbgE78ZJHECyr PoeMJh0p1htXPQfcpIXsry3DUuISiICQkm0kFszdGIkl/XXKdL595gXpy1MrOv4+gXpdf8l7zGa aMEav2xombHihoI5+kKQVLzEJjsAM/0= X-Received: by 2002:a7b:c459:0:b0:434:ff30:a165 with SMTP id 5b1f17b1804b1-436712441e2mr5994545e9.8.1734703896339; Fri, 20 Dec 2024 06:11:36 -0800 (PST) X-Google-Smtp-Source: AGHT+IFvm84P8soawQWG7WWO+ebyxGNN7WWXWb2n1BSpHTBIQgvoW1jO1ClH+ywj0Fo/WaCNkr7iYw== X-Received: by 2002:a7b:c459:0:b0:434:ff30:a165 with SMTP id 5b1f17b1804b1-436712441e2mr5994205e9.8.1734703895893; Fri, 20 Dec 2024 06:11:35 -0800 (PST) Received: from ?IPv6:2001:16b8:3db8:2e00:4b6c:c773:a3e0:8035? ([2001:16b8:3db8:2e00:4b6c:c773:a3e0:8035]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4366127c4fcsm47460175e9.29.2024.12.20.06.11.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 Dec 2024 06:11:35 -0800 (PST) Message-ID: <46f22193d960c0a0960c2ceaa525e9ff57fc09b6.camel@redhat.com> Subject: Re: [PATCH] drm/sched: Document run_job() refcount hazard From: Philipp Stanner To: Christian =?ISO-8859-1?Q?K=F6nig?= , Danilo Krummrich Cc: Philipp Stanner , Luben Tuikov , Matthew Brost , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Sumit Semwal , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, Tvrtko Ursulin , Andrey Grodzovsky Date: Fri, 20 Dec 2024 15:11:34 +0100 In-Reply-To: References: <20241220124515.93169-2-phasta@kernel.org> <5c4c610e-26ec-447c-b4db-ad38e994720b@amd.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.52.4 (3.52.4-2.fc40) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Fri, 2024-12-20 at 14:25 +0100, Christian K=C3=B6nig wrote: > Am 20.12.24 um 14:18 schrieb Danilo Krummrich: > > On Fri, Dec 20, 2024 at 01:53:34PM +0100, Christian K=C3=B6nig wrote: > > > Am 20.12.24 um 13:45 schrieb Philipp Stanner: > > > > From: Philipp Stanner > > > >=20 > > > > drm_sched_backend_ops.run_job() returns a dma_fence for the > > > > scheduler. > > > > That fence is signalled by the driver once the hardware > > > > completed the > > > > associated job. The scheduler does not increment the reference > > > > count on > > > > that fence, but implicitly expects to inherit this fence from > > > > run_job(). > > > >=20 > > > > This is relatively subtle and prone to misunderstandings. > > > >=20 > > > > This implies that, to keep a reference for itself, a driver > > > > needs to > > > > call dma_fence_get() in addition to dma_fence_init() in that > > > > callback. > > > >=20 > > > > It's further complicated by the fact that the scheduler even > > > > decrements > > > > the refcount in drm_sched_run_job_work() since it created a new > > > > reference in drm_sched_fence_scheduled(). It does, however, > > > > still use > > > > its pointer to the fence after calling dma_fence_put() - which > > > > is safe > > > > because of the aforementioned new reference, but actually still > > > > violates > > > > the refcounting rules. > > > >=20 > > > > Improve the explanatory comment for that decrement. > > > >=20 > > > > Move the call to dma_fence_put() to the position behind the > > > > last usage > > > > of the fence. > > > >=20 > > > > Document the necessity to increment the reference count in > > > > drm_sched_backend_ops.run_job(). > > > >=20 > > > > Cc: Christian K=C3=B6nig > > > > Cc: Tvrtko Ursulin > > > > Cc: Andrey Grodzovsky > > > > Signed-off-by: Philipp Stanner > > > > --- > > > > =C2=A0=C2=A0 drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++--- > > > > =C2=A0=C2=A0 include/drm/gpu_scheduler.h=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 20 > > > > ++++++++++++++++---- > > > > =C2=A0=C2=A0 2 files changed, 23 insertions(+), 7 deletions(-) > > > >=20 > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > index 7ce25281c74c..d6f8df39d848 100644 > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > + * > > > > + * @sched_job: the job to run > > > > + * > > > > + * Returns: dma_fence the driver must signal once the > > > > hardware has > > > > + * completed the job ("hardware fence"). > > > > + * > > > > + * Note that the scheduler expects to 'inherit' its > > > > own reference to > > > > + * this fence from the callback. It does not invoke an > > > > extra > > > > + * dma_fence_get() on it. Consequently, this callback > > > > must return a > > > > + * fence whose refcount is at least 2: One for the > > > > scheduler's > > > > + * reference returned here, another one for the > > > > reference kept by the > > > > + * driver. > > > Well the driver actually doesn't need any extra reference. The > > > scheduler > > > just needs to guarantee that this reference isn't dropped before > > > it is > > > signaled. > > I think he means the reference the driver's fence context has to > > have in order > > to signal that thing eventually. >=20 > Yeah, but this is usually a weak reference. IIRC most drivers don't=20 > increment the reference count for the reference they keep to signal a > fence. >=20 > It's expected that the consumers of the dma_fence keep the fence > alive=20 > at least until it is signaled. So are you saying that the driver having an extra reference (without having obtained it with dma_fence_get()) is not an issue because the driver is the one who will signal the fence [and then be done with it]? > That's why we have this nice warning in=20 > dma_fence_release(). >=20 > On the other hand I completely agree it would be more defensive if=20 > drivers increment the reference count for the reference they keep for > signaling. >=20 > So if we want to document that the fence reference count should at > least=20 > be 2 we somehow need to enforce this with a warning for example. We could =E2=80=93 but I'm not sure whether it really needs to be "enforced= ", especially if it were only to be a minor issue, as you seem to hint at above. Document it is the minimum IMO P. >=20 > Regards, > Christian. >=20 >=20 >=20 > >=20 > > > Regards, > > > Christian. > > >=20 > > > > =C2=A0=C2=A0=C2=A0 */ > > > > =C2=A0=C2=A0=C2=A0 struct dma_fence *(*run_job)(struct drm_sched_jo= b > > > > *sched_job); >=20