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....@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 >>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau