Re: [PATCH v2 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers

From: Daniel Vetter <daniel@ffwll.ch>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: Daniel Stone <daniel@fooishbar.org>,
	Eric Anholt <eric@anholt.net>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	Eben Upton <eben@raspberrypi.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v2 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers
Date: Fri, 29 Mar 2019 16:25:02 +0100	[thread overview]
Message-ID: <20190329152502.GO2665@phenom.ffwll.local> (raw)
In-Reply-To: <5ed7c5f361bca47d3f9771f9ed27e28e2fccb179.camel@bootlin.com>

On Fri, Mar 29, 2019 at 04:02:23PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri, 2019-03-29 at 09:09 +0000, Daniel Stone wrote:
> > Hi,
> > 
> > On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > > > I don't see other options either, and using firstclose/lastopen feels
> > > > overall more readable in the driver code.
> > > > 
> > > > I'm not sure there is such a big overhead associated with allocating
> > > > the binner BO (it seems that the current implementation tries to alloc
> > > > until the specific memory constraints for the buffer are met, so
> > > > perhaps that can take time). But if there is, I suppose it's best to
> > > > have that when starting up rather than delaying the first render
> > > > operation.
> > > 
> > > I'm not entirely buying the "we don't need this for fbcon only" argument -
> > > there's plenty of dumb kms clients too (boot splash and whatever else
> > > there might be). If you don't want to keep this around I think allocating
> > > on first non-dumb bo allocation and dropping it when the last such fd
> > > closes sounds like a much better idea. Needs a bit more state, you need to
> > > track per drm_file whether you've already allocated a non-dumb bo, and a
> > > drm_device refcount, but that's not much. Firstopen feels like the wrong
> > > thing.
> > > 
> > > Another option would be first_renderopen or something like that, except
> > > you can also render on the legacy node and I'm not sure how much there's a
> > > demand for this in other drivers. In the end you have open/close
> > > callbacks, you can do all the driver specific things you want to do in
> > > there.
> > 
> > I'd like to avoid doing it in open where possible, since that hurts
> > device enumeration from userspace.
> 
> I've noticed the same issue with firstopen, where our buffer is
> allocated/liberated a couple of times during enumeration, before the
> final open that stays alive during use.
> 
> I'm not sure what is preferable between that and allocating when the
> GPU is first used. Slowing down the first GPU operation with the
> allocation does not sound too great either and it feels like the buffer
> should have been allocated earlier.
> 
> To me, it feels I think it's better to have delays due to allocation at
> enumeration / startup rather than later on, but I might be missing some
> elements to have a clear idea.
> 
> What do you think?

We'll have the delay somewhere on driver load. Better to have it only once
(when the driver starts using gem for real), than a bunch of time, at
least once while enumerating and then once more while actually
initializing. I think if you allocat this on first non-dumb gem_create,
and on first command submission (just so evil userspace can't screw up the
hw too badly), that should be perfectly fine.

Only way to avoid that is to allocate at driver load and pin it, but
that's what we're trying to avoid here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch