On 9/12/23 16:28, Boris Brezillon wrote:
On Thu, 17 Aug 2023 13:13:31 +0200
Danilo Krummrich <dakr@xxxxxxxxxx> 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
