* userptr support in drm drivers
@ 2016-02-16 19:58 Jean Delvare
2016-02-16 20:33 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Jean Delvare @ 2016-02-16 19:58 UTC (permalink / raw
To: Alex Deucher, Christian König, Chris Wilson; +Cc: Daniel Vetter, dri-devel
Hi all,
While checking the openSUSE kernel configuration, I noticed a couple
oddities regarding userptr support in the i915, radeon and amdgpu drm
drivers. I'll like to discuss the current situation and come up with an
agreement on how this could be cleaned up.
Firstly, i915. This driver has userptr code under #if
defined(CONFIG_MMU_NOTIFIER), however it neither selects this option nor
depends on it. Given that CONFIG_MMU_NOTIFIER is not a user-visible
option (it can only be selected by other kernel configuration options),
it means that you get full userptr support or not depending on other
unrelated kernel options. This isn't good.
Secondly, radeon and amdgpu. They are slightly better in that they have
Kconfig options selecting CONFIG_MMU_NOTIFIER (DRM_RADEON_USERPTR and
DRM_AMDGPU_USERPTR respectively.) 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.
I am not too familiar with MMU and this userptr stuff, but from where I
stand, only two approaches make sense:
* Either there is a good reason why people may want to disable full
userptr support. In this case options CONFIG_RADEON_USERPTR and
CONFIG_AMDGPU_USERPTR should really enable the code in question, it
should not be built without these options. And a similar option
should be introduced for the i915 driver to the same effect. Or
alternatively a single option for the whole DRM subsystem may make
more sense, as I doubt anyone would want to enable support in one
driver and disable it in another.
* Or there is no specific reason why people would want to disable full
userptr support, in which case options CONFIG_RADEON_USERPTR and
CONFIG_AMDGPU_USERPTR should be removed and all 3 drivers should
unconditionally select CONFIG_MMU_NOTIFIER.
If the sole purpose of these these settings is for development or
debugging purposes, I'd go for option 1 with a run-time option to
disable full userptr (drm.userptr=ro or some such.)
As a general rule, fewer configuration options is better.
Once a decision is made, I volunteer to write the patches.
Thanks,
--
Jean Delvare
SUSE L3 Support
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-16 19:58 userptr support in drm drivers Jean Delvare
@ 2016-02-16 20:33 ` Christian König
2016-02-16 21:14 ` Daniel Vetter
2016-02-18 8:49 ` Jean Delvare
0 siblings, 2 replies; 9+ messages in thread
From: Christian König @ 2016-02-16 20:33 UTC (permalink / raw
To: Jean Delvare, Alex Deucher, Chris Wilson; +Cc: Daniel Vetter, dri-devel
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.
Regards,
Christian.
Am 16.02.2016 um 20:58 schrieb Jean Delvare:
> Hi all,
>
> While checking the openSUSE kernel configuration, I noticed a couple
> oddities regarding userptr support in the i915, radeon and amdgpu drm
> drivers. I'll like to discuss the current situation and come up with an
> agreement on how this could be cleaned up.
>
> Firstly, i915. This driver has userptr code under #if
> defined(CONFIG_MMU_NOTIFIER), however it neither selects this option nor
> depends on it. Given that CONFIG_MMU_NOTIFIER is not a user-visible
> option (it can only be selected by other kernel configuration options),
> it means that you get full userptr support or not depending on other
> unrelated kernel options. This isn't good.
>
> Secondly, radeon and amdgpu. They are slightly better in that they have
> Kconfig options selecting CONFIG_MMU_NOTIFIER (DRM_RADEON_USERPTR and
> DRM_AMDGPU_USERPTR respectively.) 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.
>
> I am not too familiar with MMU and this userptr stuff, but from where I
> stand, only two approaches make sense:
>
> * Either there is a good reason why people may want to disable full
> userptr support. In this case options CONFIG_RADEON_USERPTR and
> CONFIG_AMDGPU_USERPTR should really enable the code in question, it
> should not be built without these options. And a similar option
> should be introduced for the i915 driver to the same effect. Or
> alternatively a single option for the whole DRM subsystem may make
> more sense, as I doubt anyone would want to enable support in one
> driver and disable it in another.
>
> * Or there is no specific reason why people would want to disable full
> userptr support, in which case options CONFIG_RADEON_USERPTR and
> CONFIG_AMDGPU_USERPTR should be removed and all 3 drivers should
> unconditionally select CONFIG_MMU_NOTIFIER.
>
> If the sole purpose of these these settings is for development or
> debugging purposes, I'd go for option 1 with a run-time option to
> disable full userptr (drm.userptr=ro or some such.)
>
> As a general rule, fewer configuration options is better.
>
> Once a decision is made, I volunteer to write the patches.
>
> Thanks,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-16 20:33 ` Christian König
@ 2016-02-16 21:14 ` Daniel Vetter
2016-02-18 8:59 ` Jean Delvare
2016-02-18 8:49 ` Jean Delvare
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2016-02-16 21:14 UTC (permalink / raw
To: Christian König; +Cc: Alex Deucher, Daniel Vetter, dri-devel, Jean Delvare
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.
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.
Thanks, Daniel
>
> Regards,
> Christian.
>
> Am 16.02.2016 um 20:58 schrieb Jean Delvare:
> >Hi all,
> >
> >While checking the openSUSE kernel configuration, I noticed a couple
> >oddities regarding userptr support in the i915, radeon and amdgpu drm
> >drivers. I'll like to discuss the current situation and come up with an
> >agreement on how this could be cleaned up.
> >
> >Firstly, i915. This driver has userptr code under #if
> >defined(CONFIG_MMU_NOTIFIER), however it neither selects this option nor
> >depends on it. Given that CONFIG_MMU_NOTIFIER is not a user-visible
> >option (it can only be selected by other kernel configuration options),
> >it means that you get full userptr support or not depending on other
> >unrelated kernel options. This isn't good.
> >
> >Secondly, radeon and amdgpu. They are slightly better in that they have
> >Kconfig options selecting CONFIG_MMU_NOTIFIER (DRM_RADEON_USERPTR and
> >DRM_AMDGPU_USERPTR respectively.) 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.
> >
> >I am not too familiar with MMU and this userptr stuff, but from where I
> >stand, only two approaches make sense:
> >
> >* Either there is a good reason why people may want to disable full
> > userptr support. In this case options CONFIG_RADEON_USERPTR and
> > CONFIG_AMDGPU_USERPTR should really enable the code in question, it
> > should not be built without these options. And a similar option
> > should be introduced for the i915 driver to the same effect. Or
> > alternatively a single option for the whole DRM subsystem may make
> > more sense, as I doubt anyone would want to enable support in one
> > driver and disable it in another.
> >
> >* Or there is no specific reason why people would want to disable full
> > userptr support, in which case options CONFIG_RADEON_USERPTR and
> > CONFIG_AMDGPU_USERPTR should be removed and all 3 drivers should
> > unconditionally select CONFIG_MMU_NOTIFIER.
> >
> >If the sole purpose of these these settings is for development or
> >debugging purposes, I'd go for option 1 with a run-time option to
> >disable full userptr (drm.userptr=ro or some such.)
> >
> >As a general rule, fewer configuration options is better.
> >
> >Once a decision is made, I volunteer to write the patches.
> >
> >Thanks,
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-16 20:33 ` Christian König
2016-02-16 21:14 ` Daniel Vetter
@ 2016-02-18 8:49 ` Jean Delvare
1 sibling, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2016-02-18 8:49 UTC (permalink / raw
To: Christian König; +Cc: dri-devel, Alex Deucher, Daniel Vetter
Hi Christian,
On Tue, 16 Feb 2016 21:33:51 +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.
Again, how is this any better than just selecting MMU_NOTIFIER unconditionally?
The proliferation of kconfig options is not helping.
> 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.
--
Jean Delvare
SUSE L3 Support
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-16 21:14 ` Daniel Vetter
@ 2016-02-18 8:59 ` Jean Delvare
2016-02-18 9:48 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Jean Delvare @ 2016-02-18 8:59 UTC (permalink / raw
To: Daniel Vetter
Cc: Alex Deucher, Daniel Vetter, Christian König, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-18 8:59 ` Jean Delvare
@ 2016-02-18 9:48 ` Christian König
2016-02-19 14:11 ` Jean Delvare
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2016-02-18 9:48 UTC (permalink / raw
To: Jean Delvare, Daniel Vetter; +Cc: Alex Deucher, Daniel Vetter, dri-devel
Hi Jean,
Am 18.02.2016 um 09:59 schrieb Jean Delvare:
> 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.)
As I said, that is perfectly fine and exactly what we want.
> 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.
Adding the MMU Notifier causes overhead in the MM which people want to
avoid when they don't need userptr support.
We used to select MMU_NOTIFIER unconditionally before and explicitly
added this option on user request. So yes it is clearly necessary.
The additional userptr support code in the drivers are negligible so
always enabling the code when the MMU Notifier is available is also
perfectly fine.
So if you want to clean it up make a common DRM option which selects MMU
Notifier if it isn't already selected.
Regards,
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-18 9:48 ` Christian König
@ 2016-02-19 14:11 ` Jean Delvare
2016-02-19 14:58 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Jean Delvare @ 2016-02-19 14:11 UTC (permalink / raw
To: Christian König; +Cc: Alex Deucher, Daniel Vetter, dri-devel
Hi Christian,
On Thu, 18 Feb 2016 10:48:18 +0100, Christian König wrote:
> Am 18.02.2016 um 09:59 schrieb Jean Delvare:
> > 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.)
>
> As I said, that is perfectly fine and exactly what we want.
I don't know what "we" represent in your sentence, but I doubt a
majority of users want that.
> > 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.
>
> Adding the MMU Notifier causes overhead in the MM which people want to
> avoid when they don't need userptr support.
>
> We used to select MMU_NOTIFIER unconditionally before and explicitly
> added this option on user request. So yes it is clearly necessary.
Thanks for the explanation.
Can you tell me more about the overhead? Is it actually measurable?
Which people need this degree of optimization?
Are the i915, radeon or amdgpu drm drivers used on embedded systems?
Just because people asked for something doesn't mean that implementing
it is a good idea. Keeping things simple is a valid goal on its own.
Anyone could ask for features they don't need to become options they
can disable, but in the long run this approach can't be sustained.
> The additional userptr support code in the drivers are negligible so
> always enabling the code when the MMU Notifier is available is also
> perfectly fine.
Indeed this is a small amount of code. OTOH you have ifdefs in place
anyway. Using the right ones would turn the option into a real option,
i.e. the user decides to enable a feature or not. Not a "you may or may
not get the feature" option as we have at the moment. More control to
the user and less confusion.
> So if you want to clean it up make a common DRM option which selects MMU
> Notifier if it isn't already selected.
I will, as it will still be an improvement compared to the current
situation. But I still believe this is confusing in its current form.
If a minority of users need to opt out from MMU_NOTIFIER, then
DRM_RADEON_USERPTR and friends should default to y and be hidden behind
EXPERT. That way you avoid asking yet more questions to everyone else.
Would that work for you?
--
Jean Delvare
SUSE L3 Support
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-19 14:11 ` Jean Delvare
@ 2016-02-19 14:58 ` Christian König
2016-02-29 15:35 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2016-02-19 14:58 UTC (permalink / raw
To: Jean Delvare; +Cc: Alex Deucher, Daniel Vetter, dri-devel
> If a minority of users need to opt out from MMU_NOTIFIER, then
> DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> EXPERT. That way you avoid asking yet more questions to everyone else.
> Would that work for you?
No, just the other way around.
When MMU_NOTIFIER is selected anyway we actually don't want to display
the option, but that's not possible for none menu entries IIRC the
Kconfig correctly.
If somebody configures the kernel and don't selects any of the other
dependencies for MMU_NOTIFIER the feature should not be available by
default.
Making it an expert option is fine with me.
Regards,
Christian.
Am 19.02.2016 um 15:11 schrieb Jean Delvare:
> Hi Christian,
>
> On Thu, 18 Feb 2016 10:48:18 +0100, Christian König wrote:
>> Am 18.02.2016 um 09:59 schrieb Jean Delvare:
>>> 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.)
>> As I said, that is perfectly fine and exactly what we want.
> I don't know what "we" represent in your sentence, but I doubt a
> majority of users want that.
>
>>> 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.
>> Adding the MMU Notifier causes overhead in the MM which people want to
>> avoid when they don't need userptr support.
>>
>> We used to select MMU_NOTIFIER unconditionally before and explicitly
>> added this option on user request. So yes it is clearly necessary.
> Thanks for the explanation.
>
> Can you tell me more about the overhead? Is it actually measurable?
> Which people need this degree of optimization?
>
> Are the i915, radeon or amdgpu drm drivers used on embedded systems?
>
> Just because people asked for something doesn't mean that implementing
> it is a good idea. Keeping things simple is a valid goal on its own.
> Anyone could ask for features they don't need to become options they
> can disable, but in the long run this approach can't be sustained.
>
>> The additional userptr support code in the drivers are negligible so
>> always enabling the code when the MMU Notifier is available is also
>> perfectly fine.
> Indeed this is a small amount of code. OTOH you have ifdefs in place
> anyway. Using the right ones would turn the option into a real option,
> i.e. the user decides to enable a feature or not. Not a "you may or may
> not get the feature" option as we have at the moment. More control to
> the user and less confusion.
>
>> So if you want to clean it up make a common DRM option which selects MMU
>> Notifier if it isn't already selected.
> I will, as it will still be an improvement compared to the current
> situation. But I still believe this is confusing in its current form.
>
> If a minority of users need to opt out from MMU_NOTIFIER, then
> DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> EXPERT. That way you avoid asking yet more questions to everyone else.
> Would that work for you?
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: userptr support in drm drivers
2016-02-19 14:58 ` Christian König
@ 2016-02-29 15:35 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2016-02-29 15:35 UTC (permalink / raw
To: Christian König; +Cc: dri-devel, Alex Deucher, Daniel Vetter, Jean Delvare
On Fri, Feb 19, 2016 at 03:58:24PM +0100, Christian König wrote:
> >If a minority of users need to opt out from MMU_NOTIFIER, then
> >DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> >EXPERT. That way you avoid asking yet more questions to everyone else.
> >Would that work for you?
> No, just the other way around.
>
> When MMU_NOTIFIER is selected anyway we actually don't want to display the
> option, but that's not possible for none menu entries IIRC the Kconfig
> correctly.
>
> If somebody configures the kernel and don't selects any of the other
> dependencies for MMU_NOTIFIER the feature should not be available by
> default.
>
> Making it an expert option is fine with me.
Concurred, if we do this across all drivers supporting userptr.
-Daniel
>
> Regards,
> Christian.
>
> Am 19.02.2016 um 15:11 schrieb Jean Delvare:
> >Hi Christian,
> >
> >On Thu, 18 Feb 2016 10:48:18 +0100, Christian König wrote:
> >>Am 18.02.2016 um 09:59 schrieb Jean Delvare:
> >>>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.)
> >>As I said, that is perfectly fine and exactly what we want.
> >I don't know what "we" represent in your sentence, but I doubt a
> >majority of users want that.
> >
> >>>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.
> >>Adding the MMU Notifier causes overhead in the MM which people want to
> >>avoid when they don't need userptr support.
> >>
> >>We used to select MMU_NOTIFIER unconditionally before and explicitly
> >>added this option on user request. So yes it is clearly necessary.
> >Thanks for the explanation.
> >
> >Can you tell me more about the overhead? Is it actually measurable?
> >Which people need this degree of optimization?
> >
> >Are the i915, radeon or amdgpu drm drivers used on embedded systems?
> >
> >Just because people asked for something doesn't mean that implementing
> >it is a good idea. Keeping things simple is a valid goal on its own.
> >Anyone could ask for features they don't need to become options they
> >can disable, but in the long run this approach can't be sustained.
> >
> >>The additional userptr support code in the drivers are negligible so
> >>always enabling the code when the MMU Notifier is available is also
> >>perfectly fine.
> >Indeed this is a small amount of code. OTOH you have ifdefs in place
> >anyway. Using the right ones would turn the option into a real option,
> >i.e. the user decides to enable a feature or not. Not a "you may or may
> >not get the feature" option as we have at the moment. More control to
> >the user and less confusion.
> >
> >>So if you want to clean it up make a common DRM option which selects MMU
> >>Notifier if it isn't already selected.
> >I will, as it will still be an improvement compared to the current
> >situation. But I still believe this is confusing in its current form.
> >
> >If a minority of users need to opt out from MMU_NOTIFIER, then
> >DRM_RADEON_USERPTR and friends should default to y and be hidden behind
> >EXPERT. That way you avoid asking yet more questions to everyone else.
> >Would that work for you?
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-29 15:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-16 19:58 userptr support in drm drivers Jean Delvare
2016-02-16 20:33 ` Christian König
2016-02-16 21:14 ` Daniel Vetter
2016-02-18 8:59 ` Jean Delvare
2016-02-18 9:48 ` Christian König
2016-02-19 14:11 ` Jean Delvare
2016-02-19 14:58 ` Christian König
2016-02-29 15:35 ` Daniel Vetter
2016-02-18 8:49 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.