Static inline DRM functions calling into GPL-only code

Tue Apr 11 16:37:57 UTC 2017

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 at ffwll.ch> wrote:
>>>> On Tue, Apr 11, 2017 at 6:14 AM, Nikhil Mahale <nmahale at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel