Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Tue, 31 Oct 2023 16:01:42 +0100
From:   Danilo Krummrich <dakr@redhat.com>
To:     Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>
Cc:     Boris Brezillon <boris.brezillon@collabora.com>, airlied@gmail.com,
        daniel@ffwll.ch, matthew.brost@intel.com, faith@gfxstrand.net,
        luben.tuikov@amd.com, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow
 control
Message-ID: <ZUEW1mxwGO3bxxGM@pollux>
References: <20231027093238.2ff8172e@collabora.com>
 <ff389793-1226-49fd-b599-07dbda0b97be@amd.com>
 <20231027093943.3f0ae992@collabora.com>
 <98988459-25a8-4ee0-89d4-cb816cbc5bef@amd.com>
 <20231027102237.0cdb85af@collabora.com>
 <190e3ab7-6440-4d41-a79f-5dd4b7d3ca52@amd.com>
 <20231027121707.414810d6@collabora.com>
 <ffb8ff87-a555-42d2-aef1-a21069282227@amd.com>
 <ZT/uPhLK7gan61Ns@pollux>
 <e9c6af32-8d2a-4f04-9c12-1170a3aa1236@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e9c6af32-8d2a-4f04-9c12-1170a3aa1236@amd.com>
Precedence: bulk

On Tue, Oct 31, 2023 at 02:20:50PM +0100, Christian K?nig wrote:
> Hi Danilo,
> 
> sorry for splitting up the mail thread. I had to fetch this mail from my
> spam folder for some reason.
> 
> Am 30.10.23 um 18:56 schrieb Danilo Krummrich:
> > Hi Christian,
> > 
> > [SNIP]
> > > > And yes, we can live with the overhead of making jobs
> > > > slightly bigger than they actually are, thus potentially delaying their
> > > > execution. It's just that I don't quite understand the rational behind
> > > > this conservatism, as I don't really see what negative impact this extra
> > > > ->update_job_credits() call in the credit checking path has, other than
> > > > the slight overhead of an if-check for drivers that don't need it.
> > >  From experience it showed that we should not make the scheduler more
> > > complicated than necessary. And I still think that the ring buffers only
> > > need to be filled enough to keep the hardware busy.
> > Right, and this callback contributes to exactly that.
> > 
> > I don't really think there is much to worry about in terms of introducing more
> > complexity. The implementation behind this callback is fairly trivial - it's
> > simply called right before we check whether a job fits on the ring, to fetch
> > the job's actual size.
> > 
> > I would agree if the implementation of that would be bulky, tricky and asking
> > for a compromise. But, since it actually is simple and straight forward I really
> > think that if we implement some kind of dynamic job-flow control it should be
> > implemented as acurate as possible rather than doing it half-baked.
> 
> Yeah, I see the intention here and can perfectly relate to it it's just that
> I have prioritize other things.

I don't see any work being required from your side for this.

> 
> Adding this callback allows for the driver to influence the job submission
> and while this might seems useful now I'm just to much of a burned child to
> do stuff like this without having a really good reason for it.

It does influence the job submission in the exact same way as the initial credit
count set through drm_sched_job_init() does. There is absolutely nothing with
this callback a driver couldn't mess up in the exact same way with the initial
credit count set through drm_sched_job_init(). Following this logic we'd need to
abandon the whole patch.

Hence, I don't really understand why you're so focused on this callback.
Especially, since it's an optional one.

> 
> > > If this here has some measurable positive effect then yeah we should
> > > probably do it, but as long as it's only nice to have I have some objections
> > > to that.
> > Can't answer this, since Nouveau doesn't support native fence waits. However, I
> > guess it depends on how many native fences a job carries. So, if we'd have two
> > jobs with each of them carrying a lot of native fences, but not a lot of actual
> > work, I can very well imagine that over-accounting can have a measureable
> > impact.
> 
> What I can imagine as well is things like the hardware or firmware is
> looking at the fullness of the ring buffer to predict how much pending work
> there is.
> 
> > I just wonder if we really want to ask for real measurements given that the
> > optimization is rather trivial and cheap and we already have enough evidence
> > that it makes sense conceptually.
> 
> Well that's the point I disagree on, this callback isn't trivial. If (for
> example) the driver returns a value larger than initially estimated for the
> job we can run into an endless loop.

I agree it doesn't make sense to increase, but it wouldn't break anything,
unless the job size starts exceeding the capacity of the ring. And this case is
handled anyway.

> 
> It's just one more thing which can go boom. At bare minimum we should check
> that the value is always decreasing.

Considering the above I still agree, such a check makes sense - gonna add one.

- Danilo

> 
> Christian.
> 
> > 
> > - Danilo
> > 
> > > Regards,
> > > Christian.
> > > 
> > > > Regards,
> > > > 
> > > > Boris