[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.
Josef Larsson
josef.lar at gmail.com
Mon Dec 18 17:42:09 UTC 2017
Sounds good to me, if we remove or modify the PCI_CLASS check in
nouveau_fbcon.c as well :).
On 2017-12-18 18:36, Karol Herbst wrote:
> I've discussed it with Ben and we actually found a better solution.
> There are just some calls inside those functions which should get NULL
> checks, nv50_mstm_register_connector and nv50_mstm_destroy_connector.
> Or at least something similiar so that the code doesn't depent on the
> fbcon object being there.
>
> On Mon, Dec 18, 2017 at 6:30 PM, Josef Larsson <josef.lar at gmail.com> wrote:
>> Without a NULL pointer safe-guard patch, I get a kernel oops when I plug
>> in external displays in my docking station, (exactly the same issue as
>> https://bugs.freedesktop.org/show_bug.cgi?id=101778) and without
>> removing or modifying the check (accepting PCI_CLASS_DISPLAY_3D in the
>> if-condition), I cannot use external displays through my docking
>> station. This is on an optimus system where I use reverse PRIME to be
>> able to use the connectors at all. Having some kind of safe-guard makes
>> sense to me, and that is available in the i915-driver of the
>> corresponding functions.
>>
>> If there are better patches to fix my two problems, I am willing to try
>> them out, because I really think this should be handled somehow.
>>
>> One note though: These patches do not make the docking station
>> experience perfect. They only make it not quite as bad. For example,
>> undocking when using external displays forces Xorg to restart, and when
>> docking without a patch for xf86-video-nouveau DDX by Ilia Mirkin
>> (https://people.freedesktop.org/~imirkin/patches/0001-drmmode-update-logic-for-dynamic-connectors-paths-an.patch),
>> the external docking station displays are not detected.
>>
>>
>> On 2017-12-14 23:59, Karol Herbst wrote:
>>> On 14 December 2017 11:55:35 p.m. GMT+01:00, Ben Skeggs <skeggsb at gmail.com> wrote:
>>>> On 14 December 2017 at 23:45, Tobias Klausmann
>>>> <tobias.johannes.klausmann at mni.thm.de> wrote:
>>>>> On 12/3/17 8:56 PM, Josef Larsson wrote:
>>>>>> Sure, I can easily split it into two commits, but I would like to
>>>> have
>>>>>> an OK on the actual code changes before splitting the patch.
>>>> I'm not actually sure this is a good idea still. As I recall, part of
>>>> the purpose of that check is to prevent nouveau taking over as the
>>>> primary fbcon when it's the secondary display in the system. ie.
>>>> Optimus system with Intel driving the display, NVIDIA GPU has no
>>>> displays attached. If nouveau loads first, at some point in history,
>>>> you'd have ended up with a blank console.
>>>>
>>>> I'm not sure if this is still the case, but it warrants checking
>>>> before making this change.
>>>>
>>>> I'm more interested in what this actually solves, and why not having
>>>> fbcon prevents external displays from being used.
>>>>
>>>> Ben.
>>>>
>>> I think the issue is we can't really put any trust in this. my kepler GPU on my laptop is a VGA device, not a 3D accelerator, even though everything is wird to intel, it is a MXM card though, so in theory it is able to drive displays.
>>>
>>> We need something else to detect if the GPU is the main one or not.
>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Josef Larsson
>>>>>>
>>>>>>
>>>>>> On 2017-11-11 01:05, Tobias Klausmann wrote:
>>>>>>> On 11/10/17 7:49 PM, Josef Larsson wrote:
>>>>>>>> Accept 3d controllers and not only VGA controllers. According to
>>>> Ilia
>>>>>>>> Mirkin,
>>>>>>>> the VGA controller check should be removed. This makes it possible
>>>>>>>> to use external connectors on a docking station (40A5) for a
>>>> Thinkpad
>>>>>>>> P51.
>>>>>>>> (See Bug 101778).
>>>>>>>>
>>>>>>>> lspci example:
>>>>>>>>
>>>>>>>> 01:00.0 3D controller: NVIDIA Corporation GM107GLM [Quadro M1200
>>>> Mobile]
>>>>>>>> (rev a2)
>>>>>>>>
>>>>>>>> Also include safe-guards to avoid NULL dereferencing of fbcon,
>>>> which is
>>>>>>>> how this bug was found.
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 3 +--
>>>>>>>> drivers/gpu/drm/nouveau/nv50_display.c | 13 +++++++++++++
>>>>>>>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>>>>> index 2b12d82aac15..6b4d374a9d82 100644
>>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>>>>> @@ -498,8 +498,7 @@ nouveau_fbcon_init(struct drm_device *dev)
>>>>>>>> int preferred_bpp;
>>>>>>>> int ret;
>>>>>>>> - if (!dev->mode_config.num_crtc ||
>>>>>>>> - (dev->pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
>>>>>>>> + if (!dev->mode_config.num_crtc)
>>>>>>>> return 0;
>>>>>>>> fbcon = kzalloc(sizeof(struct nouveau_fbdev),
>>>> GFP_KERNEL);
>>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
>>>>>>>> b/drivers/gpu/drm/nouveau/nv50_display.c
>>>>>>>> index fb47d46050ec..061daf036407 100644
>>>>>>>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>>>>>>>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>>>>>>>> @@ -3214,6 +3214,13 @@ nv50_mstm_destroy_connector(struct
>>>>>>>> drm_dp_mst_topology_mgr *mgr,
>>>>>>>> struct nouveau_drm *drm = nouveau_drm(connector->dev);
>>>>>>>> struct nv50_mstc *mstc = nv50_mstc(connector);
>>>>>>>> + if (!drm->fbcon)
>>>>>>>> + {
>>>>>>>> + NV_WARN(drm, "drm->fbcon of %s point to NULL. Will not
>>>> destroy
>>>>>>>> connector\n",
>>>>>>>> + connector->name);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> drm_connector_unregister(&mstc->connector);
>>>>>>>> drm_modeset_lock_all(drm->dev);
>>>>>>>> @@ -3229,6 +3236,12 @@ nv50_mstm_register_connector(struct
>>>> drm_connector
>>>>>>>> *connector)
>>>>>>>> {
>>>>>>>> struct nouveau_drm *drm = nouveau_drm(connector->dev);
>>>>>>>> + if (!drm->fbcon)
>>>>>>>> + {
>>>>>>>> + NV_WARN(drm, "drm->fbcon of %s point to NULL. Will not
>>>> register
>>>>>>>> connector\n",
>>>>>>>> + connector->name);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> drm_modeset_lock_all(drm->dev);
>>>>>>>> drm_fb_helper_add_one_connector(&drm->fbcon->helper,
>>>> connector);
>>>>>>>> drm_modeset_unlock_all(drm->dev);
>>>>>>>>
>>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> the patch looks OK to me, yet as noted in IRC, i'd like to have
>>>> this
>>>>>>> patch split into two and have the ->fbcon check as a precondition
>>>> to
>>>>>>> the 3D Controller part. But lets see what the other and more clever
>>>>>>> people think about it! :)
>>>>>>>
>>>>>>> Greetings,
>>>>>>>
>>>>>>> Tobias
>>>>>>>
>>>>> Ping,
>>>>>
>>>>> adding Ben Skeggs and Dave Airlied to CC, maybe this will get this
>>>> little
>>>>> one commited!
>>>>>
>>>>>
>>>>> Greetings,
>>>>>
>>>>> Tobias
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Nouveau mailing list
>>>>> Nouveau at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/nouveau
>>>> _______________________________________________
>>>> Nouveau mailing list
>>>> Nouveau at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/nouveau
>>
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 862 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20171218/99aedabf/attachment.sig>
More information about the Nouveau
mailing list