Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <df7f500b-f911-4180-adf2-5a8b8bf2b6c7@gmail.com>
Date:   Fri, 27 Oct 2023 23:51:47 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:115.0) Gecko/20100101
 Thunderbird/115.4.1
Subject: Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow
 control
Content-Language: en-CA, en-US
To:     Boris Brezillon <boris.brezillon@collabora.com>
Cc:     Danilo Krummrich <dakr@redhat.com>, matthew.brost@intel.com,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        faith@gfxstrand.net, christian.koenig@amd.com
References: <20231026161431.5934-1-dakr@redhat.com>
 <20231027102516.0e4b00ef@collabora.com>
 <a9215c37-61cd-4fbc-9f80-217daacd96bd@gmail.com>
 <20231027184143.4427edb8@collabora.com>
From:   Luben Tuikov <ltuikov89@gmail.com>
Autocrypt: addr=ltuikov89@gmail.com; keydata=
 xjMEZTohOhYJKwYBBAHaRw8BAQdAWSq76k+GsENjDTMVCy9Vr4fAO9Rb57/bPT1APnbnnRHN
 Ikx1YmVuIFR1aWtvdiA8bHR1aWtvdjg5QGdtYWlsLmNvbT7CmQQTFgoAQRYhBJkj7+VmFO9b
 eaAl10wVR5QxozSvBQJlOiE6AhsDBQkJZgGABQsJCAcCAiICBhUKCQgLAgQWAgMBAh4HAheA
 AAoJEEwVR5QxozSvSm4BAOwCpX53DTQhE20FBGlTMqKCOQyJqlMcIQ9SO1qPWX1iAQCv3vfy
 JwktF7REl1yt7IU2Sye1qmQMfJxdt9JMbMNNBs44BGU6IToSCisGAQQBl1UBBQEBB0BT9wSP
 cCE8uGe7FWo8C+nTSyWPXKTx9F0gpEnlqReRBwMBCAfCfgQYFgoAJhYhBJkj7+VmFO9beaAl
 10wVR5QxozSvBQJlOiE6AhsMBQkJZgGAAAoJEEwVR5QxozSvSsYA/2LIFjbxQ2ikbU5S0pKo
 aMDzO9eGz69uNhNWJcvIKJK6AQC9228Mqc1JeZMIyjYWr2HKYHi8S2q2/zHrSZwAWYYwDA==
In-Reply-To: <20231027184143.4427edb8@collabora.com>
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature";
 boundary="------------TqV7NTz0aDGZyZGGMAGPUxnM"
Precedence: bulk

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--------------TqV7NTz0aDGZyZGGMAGPUxnM
Content-Type: multipart/mixed; boundary="------------b0m804UtGjCwO8F33HypECbv";
 protected-headers="v1"
From: Luben Tuikov <ltuikov89@gmail.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@redhat.com>, matthew.brost@intel.com,
 linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
 faith@gfxstrand.net, christian.koenig@amd.com
Message-ID: <df7f500b-f911-4180-adf2-5a8b8bf2b6c7@gmail.com>
Subject: Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow
 control
References: <20231026161431.5934-1-dakr@redhat.com>
 <20231027102516.0e4b00ef@collabora.com>
 <a9215c37-61cd-4fbc-9f80-217daacd96bd@gmail.com>
 <20231027184143.4427edb8@collabora.com>
In-Reply-To: <20231027184143.4427edb8@collabora.com>

--------------b0m804UtGjCwO8F33HypECbv
Content-Type: multipart/mixed; boundary="------------Luda7dot3zIxE6qlYy8TW7K5"

--------------Luda7dot3zIxE6qlYy8TW7K5
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 2023-10-27 12:41, Boris Brezillon wrote:
> On Fri, 27 Oct 2023 10:32:52 -0400
> Luben Tuikov <ltuikov89@gmail.com> wrote:
>=20
>> On 2023-10-27 04:25, Boris Brezillon wrote:
>>> Hi Danilo,
>>>
>>> On Thu, 26 Oct 2023 18:13:00 +0200
>>> Danilo Krummrich <dakr@redhat.com> wrote:
>>>  =20
>>>> Currently, job flow control is implemented simply by limiting the nu=
mber
>>>> of jobs in flight. Therefore, a scheduler is initialized with a cred=
it
>>>> limit that corresponds to the number of jobs which can be sent to th=
e
>>>> hardware.
>>>>
>>>> This implies that for each job, drivers need to account for the maxi=
mum
>>>> job size possible in order to not overflow the ring buffer.
>>>>
>>>> However, there are drivers, such as Nouveau, where the job size has =
a
>>>> rather large range. For such drivers it can easily happen that job
>>>> submissions not even filling the ring by 1% can block subsequent
>>>> submissions, which, in the worst case, can lead to the ring run dry.=

>>>>
>>>> In order to overcome this issue, allow for tracking the actual job s=
ize
>>>> instead of the number of jobs. Therefore, add a field to track a job=
's
>>>> credit count, which represents the number of credits a job contribut=
es
>>>> to the scheduler's credit limit.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>>> ---
>>>> Changes in V2:
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>>   - fixed up influence on scheduling fairness due to consideration o=
f a job's
>>>>     size
>>>>     - If we reach a ready entity in drm_sched_select_entity() but ca=
n't actually
>>>>       queue a job from it due to size limitations, just give up and =
go to sleep
>>>>       until woken up due to a pending job finishing, rather than con=
tinue to try
>>>>       other entities.
>>>>   - added a callback to dynamically update a job's credits (Boris)  =

>>>
>>> This callback seems controversial. I'd suggest dropping it, so the
>>> patch can be merged. =20
>>
>> Sorry, why is it controversial? (I did read back-and-forth above, but =
it wasn't clear
>> why it is /controversial/.)
>=20
> That's a question for Christian, I guess. I didn't quite get what he
> was worried about, other than this hook introducing a new way for
> drivers to screw things up by returning funky/invalid credits (which we=


It's up to the driver--they can test, test, test, and fix their code and =
so on.
We can only do so much and shouldn't be baby-sitting drivers ad nauseam. =
Drivers
can also not define this callback. :-)

> can report with WARN_ON()s). But let's be honest, there's probably a
> hundred different ways (if not more) drivers can shoot themselves in th=
e
> foot with drm_sched already...

Yes, that's true. So there's no worries with this hook.

>=20
>>
>> I believe only drivers are privy to changes in the credit availability=
 as their
>> firmware and hardware executes new jobs and finishes others, and so th=
is "update"
>> here is essential--leaving it only to prepare_job() wouldn't quite ful=
fill the vision
>> of why the credit mechanism introduced by this patch in the first plac=
e.
>=20
> I kinda agree with you, even if I wouldn't so pessimistic as to how
> useful this patch would be without the ->update_job_credits() hook
> (it already makes the situation a lot better for Nouveau and probably
> other drivers too).

Sure, and that's a good thing.

The heart of the dynamic credit scheme this patch is introducing *is* upd=
ate_job_credits()
callback. Without it, it's just about like the current job flow-control s=
cheme we have with
varied job weights (credits). Remember, it is an optional callback and dr=
iver can choose NOT
to define it--simple. :-)

So, I'm very excited about this, and see a wide range of applications and=
 tricks drivers
can do with the credit scheme (albeit had it been an "int" bwha-ha-ha-ha =
]:-> ).

Have a good weekend everyone!
--=20
Regards,
Luben

--------------Luda7dot3zIxE6qlYy8TW7K5
Content-Type: application/pgp-keys; name="OpenPGP_0x4C15479431A334AF.asc"
Content-Disposition: attachment; filename="OpenPGP_0x4C15479431A334AF.asc"
Content-Description: OpenPGP public key
Content-Transfer-Encoding: quoted-printable

-----BEGIN PGP PUBLIC KEY BLOCK-----

xjMEZTohOhYJKwYBBAHaRw8BAQdAWSq76k+GsENjDTMVCy9Vr4fAO9Rb57/bPT1A
PnbnnRHNIkx1YmVuIFR1aWtvdiA8bHR1aWtvdjg5QGdtYWlsLmNvbT7CmQQTFgoA
QRYhBJkj7+VmFO9beaAl10wVR5QxozSvBQJlOiE6AhsDBQkJZgGABQsJCAcCAiIC
BhUKCQgLAgQWAgMBAh4HAheAAAoJEEwVR5QxozSvSm4BAOwCpX53DTQhE20FBGlT
MqKCOQyJqlMcIQ9SO1qPWX1iAQCv3vfyJwktF7REl1yt7IU2Sye1qmQMfJxdt9JM
bMNNBs44BGU6IToSCisGAQQBl1UBBQEBB0BT9wSPcCE8uGe7FWo8C+nTSyWPXKTx
9F0gpEnlqReRBwMBCAfCfgQYFgoAJhYhBJkj7+VmFO9beaAl10wVR5QxozSvBQJl
OiE6AhsMBQkJZgGAAAoJEEwVR5QxozSvSsYA/2LIFjbxQ2ikbU5S0pKoaMDzO9eG
z69uNhNWJcvIKJK6AQC9228Mqc1JeZMIyjYWr2HKYHi8S2q2/zHrSZwAWYYwDA=3D=3D
=3DqCaZ
-----END PGP PUBLIC KEY BLOCK-----

--------------Luda7dot3zIxE6qlYy8TW7K5--

--------------b0m804UtGjCwO8F33HypECbv--

--------------TqV7NTz0aDGZyZGGMAGPUxnM
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature.asc"

-----BEGIN PGP SIGNATURE-----

wnsEABYIACMWIQSZI+/lZhTvW3mgJddMFUeUMaM0rwUCZTyFUwUDAAAAAAAKCRBMFUeUMaM0ry9R
AP99ffr4HYHt8VGCMy5iROcK4QMbSPUNs06YvL6SzaTxQwEAqh4nRjiYzJKeiyxwSx34v4pieDcI
66KeJE5oT9sCpAU=
=vUY+
-----END PGP SIGNATURE-----

--------------TqV7NTz0aDGZyZGGMAGPUxnM--