[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.

[Karol Herbst]

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