Messages in this thread | | | Date | Wed, 8 Mar 2023 16:02:37 +0100 | Subject | Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback | From | Christian König <> |
| |
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 >>>>>
| |