Message-ID: <269c4fb7-60f6-6778-444d-15703febea58@amd.com>
Date:   Wed, 8 Mar 2023 16:02:37 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Content-Language: en-US
To:     Karol Herbst <kherbst@redhat.com>
Cc:     Asahi Lina <lina@asahilina.net>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        =?UTF-8?Q?Bj=c3=b6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Luben Tuikov <luben.tuikov@amd.com>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Ella Stanforth <ella@iglunix.org>,
        Faith Ekstrand <faith.ekstrand@collabora.com>,
        Mary <mary@mary.zone>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org,
        linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org,
        linux-sgx@vger.kernel.org, asahi@lists.linux.dev
References: <20230307-rust-drm-v1-0-917ff5bc80a8@asahilina.net>
 <20230307-rust-drm-v1-10-917ff5bc80a8@asahilina.net>
 <cd788ccf-0cf1-85d5-1bf8-efc259bd7e11@amd.com>
 <CACO55tsnCMQt8UW5_UCY139kpZOdNXbMkBkFfUiB12jW5UgVmA@mail.gmail.com>
 <0d6cd23b-8c9e-067d-97ff-aa35dbbcf9bf@amd.com>
 <CACO55tumNMYrcJ0LhnLfTK4DmGLHD-bt3xpXyoPe98V2wmgXQg@mail.gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <CACO55tumNMYrcJ0LhnLfTK4DmGLHD-bt3xpXyoPe98V2wmgXQg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cGpqTmxYc2dsK3Z5QTY4VUt1UUJRdzRhQ0FZU2JTeDBrUnBpZ1JhNFZMYXZ6?=
 =?utf-8?B?eE9sTVE5S3FVdWUvTXk5Mzk3TnJPNTdrZGkrTHJaZkYvT0hpZWN1cEVkaTRI?=
 =?utf-8?B?UHlNRUtQRFExS0hJd3FwdFdDQUpzM1FuVlAvbmg5anJxcTNwdGJsZS84eHho?=
 =?utf-8?B?enlTdzhpUzhnOC9ZZVlSMGR3N3BEWDdibWw1bEY4aW0vSVZFbGxTRTNZNEU0?=
 =?utf-8?B?TE9qaHZLazJUM21peUlJbm9Ha3BNWmo4VEQrWGgyaDVKNHZ2V2JQVWRtelJy?=
 =?utf-8?B?WFNnd0R4Sjd4c0cwTDhCR1NXcFRKZnVTMGQyNnFGWEpkM0FaaXZwazZwZkJE?=
 =?utf-8?B?UXoyL2dtcDZTcnQyS0g5QVlQR3VndWEzR2QwSE5xdkN5eFhpaCtiekZIRzhS?=
 =?utf-8?B?RVZmUWtucTg2Q2FvbXVxY1hmcnZOZ2U2ZmJIUHRTS3RLSjhPd1lxZFNzLzIy?=
 =?utf-8?B?b3pROGVudzRlVWFBS3Faam9yYlQ1RnYyVS8veU5NWERCQTI0SVBhMXNOQnI2?=
 =?utf-8?B?MTBjd1c5ZTl4WmZVT1BCbHVoei9iWVNWQnNYbzRwY1pBWDMrWGt3cDF1cVZa?=
 =?utf-8?B?ZGt3QXpPN0JZaDVQRzdoNGFhN2x3b3F6MnRKV2hjOEYzQmo2YS9rNzJtV1F3?=
 =?utf-8?B?UExuOStra0NlY2NWWGhidnQrUUEwL1JVM1kwNHVIeEdMbGxIc09WVVdvcXR3?=
 =?utf-8?B?UzlIVGtOTnhLa1EyZVZ6ZGF2blN4TzRibnA1NVpPTnlRdEJjTEowL0xvWms2?=
 =?utf-8?B?MjFXWVVTTUFwNzRjbEVuYjRMaFg5MDl1aHNkWFV0SW96dnQ5ZCtaVS9MQkhJ?=
 =?utf-8?B?WDdlMmxCY21wZjZqeEtPU3NSa25vSDI5ZXhLQ0VBT1dwRFZKRHZ4M3UyNXZJ?=
 =?utf-8?B?TnNCK1JNVkkvaWI4T0plR3h2NlVmdzBxcVdocmtMemd5RWtiNXdLVjU4eG5J?=
 =?utf-8?B?NGhCT1BGUjVBOUhnYXFieGluMXp2N0I2akJHSlArYVlWdEQ0bEwvNWhFUVFk?=
 =?utf-8?B?UDd0bjM1MnRwZ2UzcVNtQ01JdzdVYnNlYlR0aTVJblJSdTBzcys1MjRPT1VL?=
 =?utf-8?B?ajIrOVBYaE5ZNWg1SXQwRTBaMWIzVmZKRWEydzRXbFA5N0xjMW05aTZ5NEdn?=
 =?utf-8?B?cnJSdTltLzVDTEpkVTVMY2VyN3JCcU4wblcxRmg5c21IQ2xtekNDWWdlUitS?=
 =?utf-8?B?TFd2SXFXVW1DOGxyWEN2YXZhYllsUC80Zmc1dWhtUkZsN1M4UnMxbUMwUCts?=
 =?utf-8?B?dGF1UDFneUFTbzhnZEM0MWF6bWVzVnRmbnlZVG9jMmJ4dEpIaU9hK0U1NXJO?=
 =?utf-8?B?cGdCNnh6cUhzM1V4ZXg5bDhjWHAxVUE1SFdyWk9aM200WExaSlR5NnpsV0Rz?=
 =?utf-8?B?aXdWaThKcC8xMyswRCtyM2lpc3VPZEIrZCtKWVNJc3V0by96WEdjdzU1bXpL?=
 =?utf-8?B?SEROTDdCdXBCaDhtRU1IN1FVQ0ZMUTh4c3BvWFd2dUJvRkp4QXhiWm5WK1dk?=
 =?utf-8?B?UCtPTDRNYlduQjczRnhTYzNnVng1dVBoYVZ4dWt1V0JtemVibm9vaFd2eHJ0?=
 =?utf-8?B?QTZqbGN2Q1l2ODMvMDZMYzBnTk5GSlo2NFUvSkpMcjh4UW1MdzVEcng2Wjk5?=
 =?utf-8?B?aWFKU24yRDU5SDRuNVMvRUk3UTVDclpla2lpYjRrMXVRUTduZVk3Y05zN3B2?=
 =?utf-8?B?Tm5vbGRzSytYdEM3TmREUGJCUWRQWitWUUpveUV6QjFxZGJyTnRKTk9yS214?=
 =?utf-8?B?d2w3MVBudGVrWm5Ja3lpUXNiMGV6UHFFcjhUWG5ncjJORFdQN2xScjQweGhq?=
 =?utf-8?B?bnhGVGdRRFBLYWl1c2w5b0o2LzhaTlRtMUFpZyttUEtaTkpOS1c2YkdCN2ZH?=
 =?utf-8?B?NTN3VDNiK0NUMG9ZK0Y0a3F1eDVkVWN1ZXpVSmQ0SjB2eVVJb3pLVDVBYXhk?=
 =?utf-8?B?TUtkRW03eU5pck13U1RFeXI5R2tRQ1V3UDFGUnRzZ0V4YkIrOFplT0t4OG5J?=
 =?utf-8?B?eC9Ld0s3anFEMk8vdmxOUC81d0w5SHJEZHlEQWtYam14Q1ZiTVk0NDIwMmJi?=
 =?utf-8?B?Qk9xemZUWGdnWURrQlI0Q0FkZTFqQ0hZLzNWekRMYVM0M0IzSVdnUkM5cEhF?=
 =?utf-8?B?YndYZlo1RDd5cDRTZGJiVVMvMGwwdnpJVk1YdEVmajhhaXgzQW9RQzNGcDFy?=
 =?utf-8?Q?JT/N5WUMruiOvVwpUQ8RQdg7ApiiIwgPJpcVPRnOiovL?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a1050d3a-657a-46a7-c01e-08db1fe62cd1
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 15:02:46.1221
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QbFo1q0ryZ54L/JaWG83P22tg+NexxyUrcpKIWOOCCwpBDBVEoE4ALGYPYwB3B0f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB7007
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 08.03.23 um 15:43 schrieb Karol Herbst:
> [SNIP]
> "further"? There was no discussion at all,

Yeah, well that is exactly what I wanted to archive.

>   you just started off like
> that. If you think somebody misses that connection, you can point out
> to documentation/videos whatever so the contributor can understand
> what's wrong with an approach. You did that, so that's fine. It's just
> starting off _any_ discussion with a "Well complete NAK" is terrible
> style. I'd feel uncomfortable if that happened to me and I'm sure
> there are enough people like that that we should be more reasonable
> with our replies. Just.. don't.
>
> We are all humans here and people react negatively to such things. And
> if people do it on purpose it just makes it worse.

I completely see your point, I just don't know how to improve it.

I don't stop people like this because I want to make them uncomfortable 
but because I want to prevent further discussions on that topic.

In other words how can I make people notice that this is something 
fundamental while still being polite?

>>>> This is clearly going against the idea of having jobs only depend on
>>>> fences and nothing else which is mandatory for correct memory management.
>>>>
>>> I'm sure it's all documented and there is a design document on how
>>> things have to look like you can point out? Might help to get a better
>>> understanding on how things should be.
>> Yeah, that's the problematic part. We have documented this very
>> extensively:
>> https://www.kernel.org/doc/html/v5.9/driver-api/dma-buf.html#indefinite-dma-fences
>>
>> And both Jason and Daniel gave talks about the underlying problem and
> fyi:
> s/Jason/Faith/g

+1. I wasn't aware of that.

>> try to come up with patches to raise warnings when that happens, but
>> people still keep coming up with the same idea over and over again.
>>
> Yes, and we'll have to tell them over and over again. Nothing wrong
> with that. That's just part of maintaining such a big subsystem. And
> that's definitely not a valid reason to phrase things like above.
>
>> It's just that the technical relationship between preventing jobs from
>> running and with that preventing dma_fences from signaling and the core
>> memory management with page faults and shrinkers waiting for those
>> fences is absolutely not obvious.
>>
>> We had at least 10 different teams from different companies falling into
>> the same trap already and either the patches were rejected of hand or
>> had to painfully reverted or mitigated later on.
>>
> Sure, but that's just part of the job. And pointing out fundamental
> mistakes early on is important, but the situation won't get any better
> by being like that. Yes, we'll have to repeat the same words over and
> over again, and yes that might be annoying, but that's just how it is.

Well I have no problem explaining people why a solution doesn't work.

But what usually happens is that people don't realize that they need to 
back of from a design and completely start over.

Regards,
Christian.

>
>> Regards,
>> Christian.
>>
>>>> If the hw is busy with something you need to return the fence for this
>>>> from the prepare_job callback so that the scheduler can be notified when
>>>> the hw is available again.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>>>> ---
>>>>>     drivers/gpu/drm/scheduler/sched_main.c | 10 ++++++++++
>>>>>     include/drm/gpu_scheduler.h            |  8 ++++++++
>>>>>     2 files changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 4e6ad6e122bc..5c0add2c7546 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -1001,6 +1001,16 @@ static int drm_sched_main(void *param)
>>>>>                 if (!entity)
>>>>>                         continue;
>>>>>
>>>>> +             if (sched->ops->can_run_job) {
>>>>> +                     sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>>>>> +                     if (!sched_job) {
>>>>> +                             complete_all(&entity->entity_idle);
>>>>> +                             continue;
>>>>> +                     }
>>>>> +                     if (!sched->ops->can_run_job(sched_job))
>>>>> +                             continue;
>>>>> +             }
>>>>> +
>>>>>                 sched_job = drm_sched_entity_pop_job(entity);
>>>>>
>>>>>                 if (!sched_job) {
>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>>> index 9db9e5e504ee..bd89ea9507b9 100644
>>>>> --- a/include/drm/gpu_scheduler.h
>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>> @@ -396,6 +396,14 @@ struct drm_sched_backend_ops {
>>>>>         struct dma_fence *(*prepare_job)(struct drm_sched_job *sched_job,
>>>>>                                          struct drm_sched_entity *s_entity);
>>>>>
>>>>> +     /**
>>>>> +      * @can_run_job: Called before job execution to check whether the
>>>>> +      * hardware is free enough to run the job.  This can be used to
>>>>> +      * implement more complex hardware resource policies than the
>>>>> +      * hw_submission limit.
>>>>> +      */
>>>>> +     bool (*can_run_job)(struct drm_sched_job *sched_job);
>>>>> +
>>>>>         /**
>>>>>              * @run_job: Called to execute the job once all of the dependencies
>>>>>              * have been resolved.  This may be called multiple times, if
>>>>>