Re: [Intel-xe] [PATCH v2 1/9] drm/sched: Convert drm scheduler to use a work queue rather than kthread

[Danilo Krummrich <dakr@redhat.com>]

On 9/12/23 16:28, Boris Brezillon wrote:
> On Thu, 17 Aug 2023 13:13:31 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
> 
>> I think that's a misunderstanding. I'm not trying to say that it is
>> *always* beneficial to fill up the ring as much as possible. But I think
>> it is under certain circumstances, exactly those circumstances I
>> described for Nouveau.
>>
>> As mentioned, in Nouveau the size of a job is only really limited by the
>> ring size, which means that one job can (but does not necessarily) fill
>> up the whole ring. We both agree that this is inefficient, because it
>> potentially results into the HW run dry due to hw_submission_limit == 1.
>>
>> I recognize you said that one should define hw_submission_limit and
>> adjust the other parts of the equation accordingly, the options I see are:
>>
>> (1) Increase the ring size while keeping the maximum job size.
>> (2) Decrease the maximum job size while keeping the ring size.
>> (3) Let the scheduler track the actual job size rather than the maximum
>> job size.
>>
>> (1) results into potentially wasted ring memory, because we're not
>> always reaching the maximum job size, but the scheduler assumes so.
>>
>> (2) results into more IOCTLs from userspace for the same amount of IBs
>> and more jobs result into more memory allocations and more work being
>> submitted to the workqueue (with Matt's patches).
>>
>> (3) doesn't seem to have any of those draw backs.
>>
>> What would be your take on that?
>>
>> Actually, if none of the other drivers is interested into a more precise
>> way of keeping track of the ring utilization, I'd be totally fine to do
>> it in a driver specific way. However, unfortunately I don't see how this
>> would be possible.
> 
> I'm not entirely sure, but I think PowerVR is pretty close to your
> description: jobs size is dynamic size, and the ring buffer size is
> picked by the driver at queue initialization time. What we did was to
> set hw_submission_limit to an arbitrarily high value of 64k (we could
> have used something like ringbuf_size/min_job_size instead), and then
> have the control flow implemented with ->prepare_job() [1] (CCCB is the
> PowerVR ring buffer). This allows us to maximize ring buffer utilization
> while still allowing dynamic-size jobs.

I guess this would work, but I think it would be better to bake this in,
especially if more drivers do have this need. I already have an
implementation [1] for doing that in the scheduler. My plan was to push
that as soon as Matt sends out V3.

[1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commit/269f05d6a2255384badff8b008b3c32d640d2d95

> 
>>
>> My proposal would be to just keep the hw_submission_limit (maybe rename
>> it to submission_unit_limit) and add a submission_units field to struct
>> drm_sched_job. By default a jobs submission_units field would be 0 and
>> the scheduler would behave the exact same way as it does now.
>>
>> Accordingly, jobs with submission_units > 1 would contribute more than
>> one unit to the submission_unit_limit.
>>
>> What do you think about that?
>>
>> Besides all that, you said that filling up the ring just enough to not
>> let the HW run dry rather than filling it up entirely is desirable. Why
>> do you think so? I tend to think that in most cases it shouldn't make
>> difference.
> 
> [1]https://gitlab.freedesktop.org/frankbinns/powervr/-/blob/powervr-next/drivers/gpu/drm/imagination/pvr_queue.c#L502
>