[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.
Karol Herbst
kherbst at redhat.com
Mon Dec 18 17:36:18 UTC 2017
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
>
More information about the Nouveau
mailing list