Static inline DRM functions calling into GPL-only code

Tue Apr 11 17:33:22 UTC 2017

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 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. 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 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