Static inline DRM functions calling into GPL-only code

Static inline DRM functions calling into GPL-only code

[Nikhil Mahale]

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

Re: Static inline DRM functions calling into GPL-only code

[Daniel Vetter]

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

Re: Static inline DRM functions calling into GPL-only code

[Daniel Vetter]

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

nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code)

[Lukas Wunner]

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

Re: Static inline DRM functions calling into GPL-only code

[James Jones]

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

Re: Static inline DRM functions calling into GPL-only code

[Harry Wentland]

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

Re: Static inline DRM functions calling into GPL-only code

[James Jones]

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

Re: Static inline DRM functions calling into GPL-only code

[Harry Wentland]

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

Re: nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code)

[Andy Ritger]

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

Re: nvidia vgaarb bug (was: Re: Static inline DRM functions calling into GPL-only code)

[Lukas Wunner]

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