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....@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 <skeg...@gmail.com> >> wrote: >>> On 14 December 2017 at 23:45, Tobias Klausmann >>> <tobias.johannes.klausm...@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@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/nouveau >>> _______________________________________________ >>> Nouveau mailing list >>> Nouveau@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/nouveau > > > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau > _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau