* Static inline DRM functions calling into GPL-only code @ 2017-04-11 4:14 Nikhil Mahale 2017-04-11 5:52 ` Daniel Vetter 2017-04-11 7:24 ` nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) Lukas Wunner 0 siblings, 2 replies; 10+ messages in thread From: Nikhil Mahale @ 2017-04-11 4:14 UTC (permalink / raw) To: dri-devel Hi, My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers team. I have been working on adding DRM KMS support to our driver. The NVIDIA GPU driver package (364.12 and higher) provides a kernel module, nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM driver with the DRM subsystem of the Linux kernel and advertises KMS capability on Linux kernel v4.1 or higher, with CONFIG_DRM and CONFIG_DRM_KMS_HELPER enabled. We have been able to maintain compatibility between nvidia-drm.ko and Linux kernels from v2.6.9 to v4.10. Unfortunately with release candidates of v4.11: * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's kref implementation to use refcount_inc and refcount_dec_and_test. * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc and refcount_dec_and_test EXPORT_SYMBOL_GPL. DRM drivers call refcount_inc through static inline function callchains such as: drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() drm_crtc_commit_get() => kref_get() => refcount_inc() drm_atomic_state_put() => kref_put() => refcount_dec_and_test() drm_atomic_state_get() => kref_get() => refcount_inc() drm_gem_object_reference() => kref_get => refcount_inc() This causes nvidia-drm.ko to inadvertently pick up references to EXPORT_SYMBOL_GPL symbols. There is not interest in relaxing the export of refcount_inc, and changing the license of nvidia-drm.ko isn't viable right now. So, the remaining options we see are: * Make these static inline DRM functions EXPORT_SYMBOL instead of inline. * Make these static inline DRM functions not use kref. * Make nvidia-drm.ko not use these static inline DRM functions. None of those seem good, though the first might be least bad. Do any of those seem reasonable? -- Thanks, Nikhil Mahale _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 4:14 Static inline DRM functions calling into GPL-only code Nikhil Mahale @ 2017-04-11 5:52 ` Daniel Vetter 2017-04-11 6:20 ` Daniel Vetter 2017-04-11 7:24 ` nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) Lukas Wunner 1 sibling, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2017-04-11 5:52 UTC (permalink / raw) To: Nikhil Mahale; +Cc: dri-devel On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> wrote: > My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers > team. > > I have been working on adding DRM KMS support to our driver. The NVIDIA > GPU driver package (364.12 and higher) provides a kernel module, > nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM > driver with the DRM subsystem of the Linux kernel and advertises KMS > capability on Linux kernel v4.1 or higher, with CONFIG_DRM and > CONFIG_DRM_KMS_HELPER enabled. > > We have been able to maintain compatibility between nvidia-drm.ko and > Linux kernels from v2.6.9 to v4.10. Unfortunately > with release candidates of v4.11: > > * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's > kref implementation to use refcount_inc and refcount_dec_and_test. > * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc and > refcount_dec_and_test EXPORT_SYMBOL_GPL. > > DRM drivers call refcount_inc through static inline function callchains > such as: > > drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() > drm_crtc_commit_get() => kref_get() => refcount_inc() > > drm_atomic_state_put() => kref_put() => refcount_dec_and_test() > drm_atomic_state_get() => kref_get() => refcount_inc() > > drm_gem_object_reference() => kref_get => refcount_inc() > > This causes nvidia-drm.ko to inadvertently pick up references to > EXPORT_SYMBOL_GPL symbols. > > There is not interest in relaxing the export of refcount_inc, and > changing the license of nvidia-drm.ko isn't viable right now. > > So, the remaining options we see are: > > * Make these static inline DRM functions EXPORT_SYMBOL instead of > inline. > > * Make these static inline DRM functions not use kref. > > * Make nvidia-drm.ko not use these static inline DRM functions. > > None of those seem good, though the first might be least bad. Do any of > those seem reasonable? * Open-source the nvidia kernel driver? tbh I'm not sure how much you can still make the case that your driver is fully an independent thing if you're adopting stuff like atomic modesetting. Might be better to make all the glue/remapping code from linux atomic to the shared cross-os code at least open ... And atomic is pretty much guaranteed to change all the time anyway, we're definitely not going to make a stable kabi for you folks, so you might want to do that for practical reasons anyway. Just my 2cents, personal opinion, not reflecting intel's, not legal advice, yadayada and all that :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 5:52 ` Daniel Vetter @ 2017-04-11 6:20 ` Daniel Vetter 2017-04-11 15:15 ` James Jones 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2017-04-11 6:20 UTC (permalink / raw) To: Nikhil Mahale; +Cc: dri-devel On Tue, Apr 11, 2017 at 7:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> wrote: >> My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers >> team. >> >> I have been working on adding DRM KMS support to our driver. The NVIDIA >> GPU driver package (364.12 and higher) provides a kernel module, >> nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM >> driver with the DRM subsystem of the Linux kernel and advertises KMS >> capability on Linux kernel v4.1 or higher, with CONFIG_DRM and >> CONFIG_DRM_KMS_HELPER enabled. >> >> We have been able to maintain compatibility between nvidia-drm.ko and >> Linux kernels from v2.6.9 to v4.10. Unfortunately >> with release candidates of v4.11: >> >> * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's >> kref implementation to use refcount_inc and refcount_dec_and_test. >> * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc and >> refcount_dec_and_test EXPORT_SYMBOL_GPL. >> >> DRM drivers call refcount_inc through static inline function callchains >> such as: >> >> drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() >> drm_crtc_commit_get() => kref_get() => refcount_inc() >> >> drm_atomic_state_put() => kref_put() => refcount_dec_and_test() >> drm_atomic_state_get() => kref_get() => refcount_inc() >> >> drm_gem_object_reference() => kref_get => refcount_inc() >> >> This causes nvidia-drm.ko to inadvertently pick up references to >> EXPORT_SYMBOL_GPL symbols. >> >> There is not interest in relaxing the export of refcount_inc, and >> changing the license of nvidia-drm.ko isn't viable right now. >> >> So, the remaining options we see are: >> >> * Make these static inline DRM functions EXPORT_SYMBOL instead of >> inline. >> >> * Make these static inline DRM functions not use kref. >> >> * Make nvidia-drm.ko not use these static inline DRM functions. >> >> None of those seem good, though the first might be least bad. Do any of >> those seem reasonable? > > * Open-source the nvidia kernel driver? tbh I'm not sure how much you > can still make the case that your driver is fully an independent thing > if you're adopting stuff like atomic modesetting. Might be better to > make all the glue/remapping code from linux atomic to the shared > cross-os code at least open ... And atomic is pretty much guaranteed > to change all the time anyway, we're definitely not going to make a > stable kabi for you folks, so you might want to do that for practical > reasons anyway. > > Just my 2cents, personal opinion, not reflecting intel's, not legal > advice, yadayada and all that :-) Apparently coffee didn't work yet, so let me retry the more serious part of my reply. I'd go with a shim that essentially remaps the linux atomic to whatever cross-os datastructures and semantics you have in the blob. That also has the benefit of insulating you a bit more from upstream changes in atomic (which will happen), and enthusiasts might get around to porting to new kernels before you do. Essentially pick the architecture of amd's DAL, then fully open the glue layer. With my maintainer hat on I'm at least not inclinced to add the "is this fair use or not" hacks on upstream's side, simply because sooner or later we'll break them and then we have the angry users, instead of nvidia. And that's the wrong place for bug reports for blobs :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 6:20 ` Daniel Vetter @ 2017-04-11 15:15 ` James Jones 2017-04-11 16:09 ` Harry Wentland 0 siblings, 1 reply; 10+ messages in thread From: James Jones @ 2017-04-11 15:15 UTC (permalink / raw) To: Daniel Vetter, Nikhil Mahale; +Cc: dri-devel On 04/10/2017 11:20 PM, Daniel Vetter wrote: > On Tue, Apr 11, 2017 at 7:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> wrote: >>> My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers >>> team. >>> >>> I have been working on adding DRM KMS support to our driver. The NVIDIA >>> GPU driver package (364.12 and higher) provides a kernel module, >>> nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM >>> driver with the DRM subsystem of the Linux kernel and advertises KMS >>> capability on Linux kernel v4.1 or higher, with CONFIG_DRM and >>> CONFIG_DRM_KMS_HELPER enabled. >>> >>> We have been able to maintain compatibility between nvidia-drm.ko and >>> Linux kernels from v2.6.9 to v4.10. Unfortunately >>> with release candidates of v4.11: >>> >>> * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's >>> kref implementation to use refcount_inc and refcount_dec_and_test. >>> * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc and >>> refcount_dec_and_test EXPORT_SYMBOL_GPL. >>> >>> DRM drivers call refcount_inc through static inline function callchains >>> such as: >>> >>> drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() >>> drm_crtc_commit_get() => kref_get() => refcount_inc() >>> >>> drm_atomic_state_put() => kref_put() => refcount_dec_and_test() >>> drm_atomic_state_get() => kref_get() => refcount_inc() >>> >>> drm_gem_object_reference() => kref_get => refcount_inc() >>> >>> This causes nvidia-drm.ko to inadvertently pick up references to >>> EXPORT_SYMBOL_GPL symbols. >>> >>> There is not interest in relaxing the export of refcount_inc, and >>> changing the license of nvidia-drm.ko isn't viable right now. >>> >>> So, the remaining options we see are: >>> >>> * Make these static inline DRM functions EXPORT_SYMBOL instead of >>> inline. >>> >>> * Make these static inline DRM functions not use kref. >>> >>> * Make nvidia-drm.ko not use these static inline DRM functions. >>> >>> None of those seem good, though the first might be least bad. Do any of >>> those seem reasonable? >> >> * Open-source the nvidia kernel driver? tbh I'm not sure how much you >> can still make the case that your driver is fully an independent thing >> if you're adopting stuff like atomic modesetting. Might be better to >> make all the glue/remapping code from linux atomic to the shared >> cross-os code at least open As the original message stated, this code is already open (MIT license). Thanks, -James >> ... And atomic is pretty much guaranteed >> to change all the time anyway, we're definitely not going to make a >> stable kabi for you folks, so you might want to do that for practical >> reasons anyway. >> >> Just my 2cents, personal opinion, not reflecting intel's, not legal >> advice, yadayada and all that :-) > > Apparently coffee didn't work yet, so let me retry the more serious > part of my reply. I'd go with a shim that essentially remaps the linux > atomic to whatever cross-os datastructures and semantics you have in > the blob. That also has the benefit of insulating you a bit more from > upstream changes in atomic (which will happen), and enthusiasts might > get around to porting to new kernels before you do. Essentially pick > the architecture of amd's DAL, then fully open the glue layer. With my > maintainer hat on I'm at least not inclinced to add the "is this fair > use or not" hacks on upstream's side, simply because sooner or later > we'll break them and then we have the angry users, instead of nvidia. > And that's the wrong place for bug reports for blobs :-) > -Daniel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 15:15 ` James Jones @ 2017-04-11 16:09 ` Harry Wentland 2017-04-11 16:37 ` James Jones 0 siblings, 1 reply; 10+ messages in thread From: Harry Wentland @ 2017-04-11 16:09 UTC (permalink / raw) To: James Jones, Daniel Vetter, Nikhil Mahale; +Cc: dri-devel On 2017-04-11 11:15 AM, James Jones wrote: > On 04/10/2017 11:20 PM, Daniel Vetter wrote: >> On Tue, Apr 11, 2017 at 7:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> >>> wrote: >>>> My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers >>>> team. >>>> >>>> I have been working on adding DRM KMS support to our driver. The NVIDIA >>>> GPU driver package (364.12 and higher) provides a kernel module, >>>> nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM >>>> driver with the DRM subsystem of the Linux kernel and advertises KMS >>>> capability on Linux kernel v4.1 or higher, with CONFIG_DRM and >>>> CONFIG_DRM_KMS_HELPER enabled. >>>> >>>> We have been able to maintain compatibility between nvidia-drm.ko and >>>> Linux kernels from v2.6.9 to v4.10. Unfortunately >>>> with release candidates of v4.11: >>>> >>>> * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's >>>> kref implementation to use refcount_inc and refcount_dec_and_test. >>>> * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc and >>>> refcount_dec_and_test EXPORT_SYMBOL_GPL. >>>> >>>> DRM drivers call refcount_inc through static inline function callchains >>>> such as: >>>> >>>> drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() >>>> drm_crtc_commit_get() => kref_get() => refcount_inc() >>>> >>>> drm_atomic_state_put() => kref_put() => refcount_dec_and_test() >>>> drm_atomic_state_get() => kref_get() => refcount_inc() >>>> >>>> drm_gem_object_reference() => kref_get => refcount_inc() >>>> >>>> This causes nvidia-drm.ko to inadvertently pick up references to >>>> EXPORT_SYMBOL_GPL symbols. >>>> >>>> There is not interest in relaxing the export of refcount_inc, and >>>> changing the license of nvidia-drm.ko isn't viable right now. >>>> >>>> So, the remaining options we see are: >>>> >>>> * Make these static inline DRM functions EXPORT_SYMBOL instead of >>>> inline. >>>> >>>> * Make these static inline DRM functions not use kref. >>>> >>>> * Make nvidia-drm.ko not use these static inline DRM functions. >>>> >>>> None of those seem good, though the first might be least bad. Do >>>> any of >>>> those seem reasonable? >>> >>> * Open-source the nvidia kernel driver? tbh I'm not sure how much you >>> can still make the case that your driver is fully an independent thing >>> if you're adopting stuff like atomic modesetting. Might be better to >>> make all the glue/remapping code from linux atomic to the shared >>> cross-os code at least open > > As the original message stated, this code is already open (MIT license). > Just out of curiosity, can I find this on any public repo or webpage? If inlining is the issue it looks like this is not used by any upstream DRM driver (or DAL) directly but only from a bunch of atomic functions, none of which are inline. If this is an issue for NVidia would this also be an issue for any other MIT licensed code, such as drm_atomic_helper.c? Harry > Thanks, > -James > >>> ... And atomic is pretty much guaranteed >>> to change all the time anyway, we're definitely not going to make a >>> stable kabi for you folks, so you might want to do that for practical >>> reasons anyway. >>> >>> Just my 2cents, personal opinion, not reflecting intel's, not legal >>> advice, yadayada and all that :-) >> >> Apparently coffee didn't work yet, so let me retry the more serious >> part of my reply. I'd go with a shim that essentially remaps the linux >> atomic to whatever cross-os datastructures and semantics you have in >> the blob. That also has the benefit of insulating you a bit more from >> upstream changes in atomic (which will happen), and enthusiasts might >> get around to porting to new kernels before you do. Essentially pick >> the architecture of amd's DAL, then fully open the glue layer. With my >> maintainer hat on I'm at least not inclinced to add the "is this fair >> use or not" hacks on upstream's side, simply because sooner or later >> we'll break them and then we have the angry users, instead of nvidia. >> And that's the wrong place for bug reports for blobs :-) >> -Daniel >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 16:09 ` Harry Wentland @ 2017-04-11 16:37 ` James Jones 2017-04-11 17:33 ` Harry Wentland 0 siblings, 1 reply; 10+ messages in thread From: James Jones @ 2017-04-11 16:37 UTC (permalink / raw) To: Harry Wentland, Daniel Vetter, Nikhil Mahale; +Cc: dri-devel On 04/11/2017 09:09 AM, Harry Wentland wrote: > On 2017-04-11 11:15 AM, James Jones wrote: >> On 04/10/2017 11:20 PM, Daniel Vetter wrote: >>> On Tue, Apr 11, 2017 at 7:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> >>>> wrote: >>>>> My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers >>>>> team. >>>>> >>>>> I have been working on adding DRM KMS support to our driver. The >>>>> NVIDIA >>>>> GPU driver package (364.12 and higher) provides a kernel module, >>>>> nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM >>>>> driver with the DRM subsystem of the Linux kernel and advertises KMS >>>>> capability on Linux kernel v4.1 or higher, with CONFIG_DRM and >>>>> CONFIG_DRM_KMS_HELPER enabled. >>>>> >>>>> We have been able to maintain compatibility between nvidia-drm.ko and >>>>> Linux kernels from v2.6.9 to v4.10. Unfortunately >>>>> with release candidates of v4.11: >>>>> >>>>> * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the kernel's >>>>> kref implementation to use refcount_inc and refcount_dec_and_test. >>>>> * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc >>>>> and >>>>> refcount_dec_and_test EXPORT_SYMBOL_GPL. >>>>> >>>>> DRM drivers call refcount_inc through static inline function >>>>> callchains >>>>> such as: >>>>> >>>>> drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() >>>>> drm_crtc_commit_get() => kref_get() => refcount_inc() >>>>> >>>>> drm_atomic_state_put() => kref_put() => refcount_dec_and_test() >>>>> drm_atomic_state_get() => kref_get() => refcount_inc() >>>>> >>>>> drm_gem_object_reference() => kref_get => refcount_inc() >>>>> >>>>> This causes nvidia-drm.ko to inadvertently pick up references to >>>>> EXPORT_SYMBOL_GPL symbols. >>>>> >>>>> There is not interest in relaxing the export of refcount_inc, and >>>>> changing the license of nvidia-drm.ko isn't viable right now. >>>>> >>>>> So, the remaining options we see are: >>>>> >>>>> * Make these static inline DRM functions EXPORT_SYMBOL instead of >>>>> inline. >>>>> >>>>> * Make these static inline DRM functions not use kref. >>>>> >>>>> * Make nvidia-drm.ko not use these static inline DRM functions. >>>>> >>>>> None of those seem good, though the first might be least bad. Do >>>>> any of >>>>> those seem reasonable? >>>> >>>> * Open-source the nvidia kernel driver? tbh I'm not sure how much you >>>> can still make the case that your driver is fully an independent thing >>>> if you're adopting stuff like atomic modesetting. Might be better to >>>> make all the glue/remapping code from linux atomic to the shared >>>> cross-os code at least open >> >> As the original message stated, this code is already open (MIT license). >> > > Just out of curiosity, can I find this on any public repo or webpage? This is our usual Linux driver download landing page: https://www.nvidia.com/object/unix.html We don't break out the nvidia-drm source into a separate package like we do for some of our other open-source components, but it's included when you download the full driver. You can unpack it without installing, e.g: $ sh ~/Downloads/NVIDIA-Linux-x86_64-378.13.run -x Then it will be in ./NVIDIA-Linux-x86_64-378.13/kernel/nvidia-drm/ Feedback welcome. Thanks, -James > If inlining is the issue it looks like this is not used by any upstream > DRM driver (or DAL) directly but only from a bunch of atomic functions, > none of which are inline. > > If this is an issue for NVidia would this also be an issue for any other > MIT licensed code, such as drm_atomic_helper.c? > > Harry > >> Thanks, >> -James >> >>>> ... And atomic is pretty much guaranteed >>>> to change all the time anyway, we're definitely not going to make a >>>> stable kabi for you folks, so you might want to do that for practical >>>> reasons anyway. >>>> >>>> Just my 2cents, personal opinion, not reflecting intel's, not legal >>>> advice, yadayada and all that :-) >>> >>> Apparently coffee didn't work yet, so let me retry the more serious >>> part of my reply. I'd go with a shim that essentially remaps the linux >>> atomic to whatever cross-os datastructures and semantics you have in >>> the blob. That also has the benefit of insulating you a bit more from >>> upstream changes in atomic (which will happen), and enthusiasts might >>> get around to porting to new kernels before you do. Essentially pick >>> the architecture of amd's DAL, then fully open the glue layer. With my >>> maintainer hat on I'm at least not inclinced to add the "is this fair >>> use or not" hacks on upstream's side, simply because sooner or later >>> we'll break them and then we have the angry users, instead of nvidia. >>> And that's the wrong place for bug reports for blobs :-) >>> -Daniel >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Static inline DRM functions calling into GPL-only code 2017-04-11 16:37 ` James Jones @ 2017-04-11 17:33 ` Harry Wentland 0 siblings, 0 replies; 10+ messages in thread From: Harry Wentland @ 2017-04-11 17:33 UTC (permalink / raw) To: James Jones, Daniel Vetter, Nikhil Mahale; +Cc: dri-devel On 2017-04-11 12:37 PM, James Jones wrote: > On 04/11/2017 09:09 AM, Harry Wentland wrote: >> On 2017-04-11 11:15 AM, James Jones wrote: >>> On 04/10/2017 11:20 PM, Daniel Vetter wrote: >>>> On Tue, Apr 11, 2017 at 7:52 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>> On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale@nvidia.com> >>>>> wrote: >>>>>> My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers >>>>>> team. >>>>>> >>>>>> I have been working on adding DRM KMS support to our driver. The >>>>>> NVIDIA >>>>>> GPU driver package (364.12 and higher) provides a kernel module, >>>>>> nvidia-drm.ko, which is licensed as "MIT". This module registers a >>>>>> DRM >>>>>> driver with the DRM subsystem of the Linux kernel and advertises KMS >>>>>> capability on Linux kernel v4.1 or higher, with CONFIG_DRM and >>>>>> CONFIG_DRM_KMS_HELPER enabled. >>>>>> >>>>>> We have been able to maintain compatibility between nvidia-drm.ko and >>>>>> Linux kernels from v2.6.9 to v4.10. Unfortunately >>>>>> with release candidates of v4.11: >>>>>> >>>>>> * Commit 10383aea2f445bce9b2a2b308def08134b438c8e changed the >>>>>> kernel's >>>>>> kref implementation to use refcount_inc and refcount_dec_and_test. >>>>>> * Commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4 made refcount_inc >>>>>> and >>>>>> refcount_dec_and_test EXPORT_SYMBOL_GPL. >>>>>> >>>>>> DRM drivers call refcount_inc through static inline function >>>>>> callchains >>>>>> such as: >>>>>> >>>>>> drm_crtc_commit_put() => kref_put() => refcount_dec_and_test() >>>>>> drm_crtc_commit_get() => kref_get() => refcount_inc() >>>>>> >>>>>> drm_atomic_state_put() => kref_put() => refcount_dec_and_test() >>>>>> drm_atomic_state_get() => kref_get() => refcount_inc() >>>>>> >>>>>> drm_gem_object_reference() => kref_get => refcount_inc() >>>>>> >>>>>> This causes nvidia-drm.ko to inadvertently pick up references to >>>>>> EXPORT_SYMBOL_GPL symbols. >>>>>> >>>>>> There is not interest in relaxing the export of refcount_inc, and >>>>>> changing the license of nvidia-drm.ko isn't viable right now. >>>>>> >>>>>> So, the remaining options we see are: >>>>>> >>>>>> * Make these static inline DRM functions EXPORT_SYMBOL instead of >>>>>> inline. >>>>>> >>>>>> * Make these static inline DRM functions not use kref. >>>>>> >>>>>> * Make nvidia-drm.ko not use these static inline DRM functions. >>>>>> >>>>>> None of those seem good, though the first might be least bad. Do >>>>>> any of >>>>>> those seem reasonable? >>>>> >>>>> * Open-source the nvidia kernel driver? tbh I'm not sure how much you >>>>> can still make the case that your driver is fully an independent thing >>>>> if you're adopting stuff like atomic modesetting. Might be better to >>>>> make all the glue/remapping code from linux atomic to the shared >>>>> cross-os code at least open >>> >>> As the original message stated, this code is already open (MIT license). >>> >> >> Just out of curiosity, can I find this on any public repo or webpage? > > This is our usual Linux driver download landing page: > > https://www.nvidia.com/object/unix.html > > We don't break out the nvidia-drm source into a separate package like we > do for some of our other open-source components, but it's included when > you download the full driver. You can unpack it without installing, e.g: > > $ sh ~/Downloads/NVIDIA-Linux-x86_64-378.13.run -x > > Then it will be in ./NVIDIA-Linux-x86_64-378.13/kernel/nvidia-drm/ > > Feedback welcome. Thanks. Looks nice. I only spent 20 mins looking at it but really like how you guys deal with changes in kernel interfaces for kernel compatibility based on indicators in the kernel source. I also like your nvidia-drm-modeset. Looks really nice and clean, at least on the surface. Harry > > Thanks, > -James > >> If inlining is the issue it looks like this is not used by any upstream >> DRM driver (or DAL) directly but only from a bunch of atomic functions, >> none of which are inline. >> >> If this is an issue for NVidia would this also be an issue for any other >> MIT licensed code, such as drm_atomic_helper.c? >> >> Harry >> >>> Thanks, >>> -James >>> >>>>> ... And atomic is pretty much guaranteed >>>>> to change all the time anyway, we're definitely not going to make a >>>>> stable kabi for you folks, so you might want to do that for practical >>>>> reasons anyway. >>>>> >>>>> Just my 2cents, personal opinion, not reflecting intel's, not legal >>>>> advice, yadayada and all that :-) >>>> >>>> Apparently coffee didn't work yet, so let me retry the more serious >>>> part of my reply. I'd go with a shim that essentially remaps the linux >>>> atomic to whatever cross-os datastructures and semantics you have in >>>> the blob. That also has the benefit of insulating you a bit more from >>>> upstream changes in atomic (which will happen), and enthusiasts might >>>> get around to porting to new kernels before you do. Essentially pick >>>> the architecture of amd's DAL, then fully open the glue layer. With my >>>> maintainer hat on I'm at least not inclinced to add the "is this fair >>>> use or not" hacks on upstream's side, simply because sooner or later >>>> we'll break them and then we have the angry users, instead of nvidia. >>>> And that's the wrong place for bug reports for blobs :-) >>>> -Daniel >>>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) 2017-04-11 4:14 Static inline DRM functions calling into GPL-only code Nikhil Mahale 2017-04-11 5:52 ` Daniel Vetter @ 2017-04-11 7:24 ` Lukas Wunner 2017-04-12 22:46 ` Andy Ritger 1 sibling, 1 reply; 10+ messages in thread From: Lukas Wunner @ 2017-04-11 7:24 UTC (permalink / raw) To: Nikhil Mahale; +Cc: dri-devel Hi Nikhil, On Tue, Apr 11, 2017 at 09:44:35AM +0530, Nikhil Mahale wrote: > There is not interest in relaxing the export of refcount_inc, and > changing the license of nvidia-drm.ko isn't viable right now. Why not dual-license as MIT+GPL? > * Make these static inline DRM functions EXPORT_SYMBOL instead of > inline. Intuitively, I'd say wrapping a function declared EXPORT_SYMBOL_GPL in another function declared EXPORT_SYMBOL doesn't seem like a legal way to solve this issue. > My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers > team. > > I have been working on adding DRM KMS support to our driver. The NVIDIA > GPU driver package (364.12 and higher) provides a kernel module, > nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM > driver with the DRM subsystem of the Linux kernel and advertises KMS > capability on Linux kernel v4.1 or higher, with CONFIG_DRM and > CONFIG_DRM_KMS_HELPER enabled. Sorry to hijack this thread, but there's an egregious, long-standing bug in your driver with regards to vgaarb usage: nvidia/nv.c calls vga_tryget() but never calls vga_put(), in other words your driver locks legacy VGA I/O but never unlocks it. This is in the proprietary, non-MIT licensed portion of your driver. The bug was already reported to Nvidia four years ago: https://devtalk.nvidia.com/default/topic/545560 It causes issues such as deadlocks or inability to control backlight on MacBook Pros, commit 4eebd5a4e726 tried to work around it but introduced more problems. Could you please look into fixing that bug? Thanks! Lukas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) 2017-04-11 7:24 ` nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) Lukas Wunner @ 2017-04-12 22:46 ` Andy Ritger 2017-12-06 11:24 ` Lukas Wunner 0 siblings, 1 reply; 10+ messages in thread From: Andy Ritger @ 2017-04-12 22:46 UTC (permalink / raw) To: Lukas Wunner; +Cc: dri-devel On Tue, Apr 11, 2017 at 09:24:33AM +0200, Lukas Wunner wrote: > Hi Nikhil, > > On Tue, Apr 11, 2017 at 09:44:35AM +0530, Nikhil Mahale wrote: > > There is not interest in relaxing the export of refcount_inc, and > > changing the license of nvidia-drm.ko isn't viable right now. > > Why not dual-license as MIT+GPL? We intentionally chose MODULE_LICENSE("MIT") for nvidia-drm.ko, rather than MODULE_LICENSE("Dual MIT/GPL"), to avoid any ambiguity: * nvidia-drm.ko depends on nvidia.ko, which is MODULE_LICENSE("NVIDIA"). * nvidia-drm.ko is the portion of the NVIDIA dGPU driver that interfaces with DRM in the kernel. We wouldn't want nvidia-drm.ko to inadvertently function as, or even be perceived as, a path for nvidia.ko to indirectly get access to EXPORT_SYMBOL_GPL symbols. > > * Make these static inline DRM functions EXPORT_SYMBOL instead of > > inline. > > Intuitively, I'd say wrapping a function declared EXPORT_SYMBOL_GPL > in another function declared EXPORT_SYMBOL doesn't seem like a legal > way to solve this issue. I understand why people may have that concern. But to me, it seems like there is a difference between refcount_dec_and_test() and drm_crtc_commit_put(). The former is a kernel-internal implementation detail, where EXPORT_SYMBOL_GPL() seems appropriate. The latter is an interface exposed by DRM, with the intent to enable DRM drivers to use it. That drm_crtc_commit_put() is one-line long and simply calls kref_put()/refcount_dec_and_test() seems like an implementation detail of drm_crtc_commit_put(), and independent of the interface DRM provides to its drivers. Maybe there are better options, but of the options Nikhil outlined: > * Make these static inline DRM functions EXPORT_SYMBOL instead of inline. > > * Make these static inline DRM functions not use kref. > > * Make nvidia-drm.ko not use these static inline DRM functions. The first one seems sort of reasonable to me, given the above reasoning. It also seems somewhat in the spirit of allowing DRM to be usable on a variety of differently-licensed platforms. To be fair, it would be unfortunate to lose the static inlineness of those functions. > > My name is Nikhil Mahale, and I work at NVIDIA in the Linux drivers > > team. > > > > I have been working on adding DRM KMS support to our driver. The NVIDIA > > GPU driver package (364.12 and higher) provides a kernel module, > > nvidia-drm.ko, which is licensed as "MIT". This module registers a DRM > > driver with the DRM subsystem of the Linux kernel and advertises KMS > > capability on Linux kernel v4.1 or higher, with CONFIG_DRM and > > CONFIG_DRM_KMS_HELPER enabled. > > Sorry to hijack this thread, but there's an egregious, long-standing bug > in your driver with regards to vgaarb usage: nvidia/nv.c calls > vga_tryget() but never calls vga_put(), in other words your driver locks > legacy VGA I/O but never unlocks it. This is in the proprietary, non-MIT > licensed portion of your driver. > > The bug was already reported to Nvidia four years ago: > https://devtalk.nvidia.com/default/topic/545560 > > It causes issues such as deadlocks or inability to control backlight > on MacBook Pros, commit 4eebd5a4e726 tried to work around it but > introduced more problems. > > Could you please look into fixing that bug? Thanks! Thanks for pointing that out. Yes, we'll take a look. Thanks, - Andy > Lukas > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) 2017-04-12 22:46 ` Andy Ritger @ 2017-12-06 11:24 ` Lukas Wunner 0 siblings, 0 replies; 10+ messages in thread From: Lukas Wunner @ 2017-12-06 11:24 UTC (permalink / raw) To: Andy Ritger; +Cc: dri-devel On Wed, Apr 12, 2017 at 03:46:05PM -0700, Andy Ritger wrote: > On Tue, Apr 11, 2017 at 09:24:33AM +0200, Lukas Wunner wrote: > > Sorry to hijack this thread, but there's an egregious, long-standing bug > > in your driver with regards to vgaarb usage: nvidia/nv.c calls > > vga_tryget() but never calls vga_put(), in other words your driver locks > > legacy VGA I/O but never unlocks it. This is in the proprietary, non-MIT > > licensed portion of your driver. > > > > The bug was already reported to Nvidia four years ago: > > https://devtalk.nvidia.com/default/topic/545560 > > > > It causes issues such as deadlocks or inability to control backlight > > on MacBook Pros, commit 4eebd5a4e726 tried to work around it but > > introduced more problems. > > > > Could you please look into fixing that bug? Thanks! > > Thanks for pointing that out. Yes, we'll take a look. Andy, eight months have passed and this bug is still present in your latest driver version 387.34. Could you please provide an ETA for a fix? Thanks, Lukas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-12-06 11:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-11 4:14 Static inline DRM functions calling into GPL-only code Nikhil Mahale 2017-04-11 5:52 ` Daniel Vetter 2017-04-11 6:20 ` Daniel Vetter 2017-04-11 15:15 ` James Jones 2017-04-11 16:09 ` Harry Wentland 2017-04-11 16:37 ` James Jones 2017-04-11 17:33 ` Harry Wentland 2017-04-11 7:24 ` nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code) Lukas Wunner 2017-04-12 22:46 ` Andy Ritger 2017-12-06 11:24 ` Lukas Wunner
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.