Josef Larsson
2017-Dec-03 19:56 UTC
[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.
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. 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 >-------------- 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/20171203/781df0d5/attachment.sig>
Tobias Klausmann
2017-Dec-14 13:45 UTC
[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.
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. > > 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
Ben Skeggs
2017-Dec-14 22:55 UTC
[Nouveau] [PATCH] Accept 3d controllers and not only VGA controllers.
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.>> >> 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
Possibly Parallel Threads
- [PATCH] Accept 3d controllers and not only VGA controllers.
- [PATCH] Accept 3d controllers and not only VGA controllers.
- [PATCH] Accept 3d controllers and not only VGA controllers.
- [PATCH] Accept 3d controllers and not only VGA controllers.
- [PATCH] Accept 3d controllers and not only VGA controllers.