Re: userptr support in drm drivers

From: Jean Delvare <jdelvare@suse.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "Alex Deucher" <alexander.deucher@amd.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Christian König" <christian.koenig@amd.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: userptr support in drm drivers
Date: Thu, 18 Feb 2016 09:59:05 +0100	[thread overview]
Message-ID: <20160218095905.2cda4fce@endymion> (raw)
In-Reply-To: <20160216211412.GC32705@phenom.ffwll.local>

Hi Daniel,

On Tue, 16 Feb 2016 22:14:12 +0100, Daniel Vetter wrote:
> On Tue, Feb 16, 2016 at 09:33:51PM +0100, Christian König wrote:
> > At least for Radeon and Amdgpu the current situation is actually what we
> > want.
> > 
> > >However I still find it confusing
> > >that full userptr support is under #if defined(CONFIG_MMU_NOTIFIER),
> > >and not under #if defined(CONFIG_RADEON_USERPTR), resp. #if
> > >defined(CONFIG_AMDGPU_USERPTR). It means that full userptr support may
> > >be included even if these options are disabled.
> > Which is perfectly fine. Userptr support should be enabled when MMU_NOTIFIER
> > is available.
> > 
> > How this becomes available is a different story. You can explicitly enable
> > it which then pulls in the MMU_NOTIFIER dependency or you just enable it
> > when you have the notfier anyway because of some other dependency.
> > 
> > That we have two options doing the same is just a matter of branching of
> > amdgpu from radeon and not cleaning up the configuration options. So feel
> > free to cleaning this up and write a patch which makes pulling in
> > MMU_NOTIFIER as a general DRM option.
> 
> Yeah I like that flow. Jean, if you want to bring i915 into alignment with
> radone by adding a I915_USERPTR option that selects MMU_NOTIFIER (probably
> default y since vulkan needs this), then I very much want will merge it.

Maybe I was not clear enough in my original post, but I am really
advocating for FEWER kconfig options, not more. Plus I just explained
why I think the radeon and amdgpu drivers do it wrong, so I'm certainly
not going to align i915 on that.

> Distro kernels pretty much all select MMU_NOTIFIER already for unrelated
> reasons, but it's good to be less suprising for everyone who builds their
> own custom kernel.

The current situation is actually very surprising and this is what I
would like to change. The options of one driver depending on choices
made somewhere else is utterly confusing. CONFIG_RADEON_USERPTR and
CONFIG_AMDGPU_USERPTR give the user a little more control where i915
gives none, but that's only in one direction (you can force full
userptr support in, but you can't force it out.)

I'm still waiting for someone to explain why we can't just
unconditionally select MMU_NOTIFIER in these 3 drivers and be done with
it. That's less work at kernel configuration time AND fewer
preprocessing conditionals within the code, so easier/better testing
coverage and more readable code. We would need a very good reason for
NOT doing it.

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel