* Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes [not found] ` <07f5e4a3-f7ef-df5e-d8cc-3759d828719d@shipmail.org> @ 2017-02-19 15:21 ` Thomas Hellstrom 2017-02-20 22:22 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Thomas Hellstrom @ 2017-02-19 15:21 UTC (permalink / raw) To: Daniel Vetter, Intel Graphics Development, DRI Development Cc: Daniel Vetter, Dave Airlie, linux-kernel@vger.kernel.org So I think we need a quick revert of this commit or a quick stable follow-up to unbreak things on our side. /Thomas On 02/19/2017 03:54 PM, Thomas Hellstrom wrote: > Hi! > > This patch breaks the vmwgfx resolutionKMS daemon which opens a control > node to tell DRM about the monitor layout... > > /Thomas > > > On 10/28/2016 10:10 AM, Daniel Vetter wrote: >> Looking at the ioctl permission checks I noticed that it's impossible >> to import gem buffers into a control nodes, and fd2handle/handle2fd >> also don't work, so no joy with dma-bufs. >> >> The only way to do anything with a control node is by drawing stuff >> into a dumb buffer and displaying that. I suspect control nodes are an >> entirely unused thing, and a cursory check shows that there does not >> seem to be any callers of drmOpenControl nor of the other drmOpen >> functions using DRM_MODE_CONTROL. >> >> Since I don't like dead uabi, let's remove it. But since this would be >> a really big change I think it's better to start out small by simply >> not registering anything. We can garbage-collect the dead code later >> on, once we're sure it's really not used anywhere. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> drivers/gpu/drm/drm_drv.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 6efdba4993fc..f085e28ffc6f 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev, >> goto err_free; >> } >> >> - if (drm_core_check_feature(dev, DRIVER_MODESET)) { >> - ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL); >> - if (ret) >> - goto err_minors; >> - } >> - >> if (drm_core_check_feature(dev, DRIVER_RENDER)) { >> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER); >> if (ret) > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes 2017-02-19 15:21 ` [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes Thomas Hellstrom @ 2017-02-20 22:22 ` Daniel Vetter 2017-02-21 5:32 ` DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) Thomas Hellstrom 2017-02-21 11:11 ` [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes Thomas Hellstrom 0 siblings, 2 replies; 6+ messages in thread From: Daniel Vetter @ 2017-02-20 22:22 UTC (permalink / raw) To: Thomas Hellstrom Cc: Intel Graphics Development, DRI Development, Daniel Vetter, Dave Airlie, linux-kernel@vger.kernel.org On Sun, Feb 19, 2017 at 4:21 PM, Thomas Hellstrom <thomas@shipmail.org> wrote: > So I think we need a quick revert of this commit or a quick stable > follow-up to unbreak things on our side. I'd much prefer we just register control nodes for vmwgfx only, with a commit message explaining in detail what exactly your control tool is using (i.e. which ioctl), plus links to the source code for future references. Also not sold on the immediate revert, this stuff has been merged since months. -Daniel > > /Thomas > > > On 02/19/2017 03:54 PM, Thomas Hellstrom wrote: >> Hi! >> >> This patch breaks the vmwgfx resolutionKMS daemon which opens a control >> node to tell DRM about the monitor layout... >> >> /Thomas >> >> >> On 10/28/2016 10:10 AM, Daniel Vetter wrote: >>> Looking at the ioctl permission checks I noticed that it's impossible >>> to import gem buffers into a control nodes, and fd2handle/handle2fd >>> also don't work, so no joy with dma-bufs. >>> >>> The only way to do anything with a control node is by drawing stuff >>> into a dumb buffer and displaying that. I suspect control nodes are an >>> entirely unused thing, and a cursory check shows that there does not >>> seem to be any callers of drmOpenControl nor of the other drmOpen >>> functions using DRM_MODE_CONTROL. >>> >>> Since I don't like dead uabi, let's remove it. But since this would be >>> a really big change I think it's better to start out small by simply >>> not registering anything. We can garbage-collect the dead code later >>> on, once we're sure it's really not used anywhere. >>> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/drm_drv.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 6efdba4993fc..f085e28ffc6f 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >>> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev, >>> goto err_free; >>> } >>> >>> - if (drm_core_check_feature(dev, DRIVER_MODESET)) { >>> - ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL); >>> - if (ret) >>> - goto err_minors; >>> - } >>> - >>> if (drm_core_check_feature(dev, DRIVER_RENDER)) { >>> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER); >>> if (ret) >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) 2017-02-20 22:22 ` Daniel Vetter @ 2017-02-21 5:32 ` Thomas Hellstrom 2017-02-21 5:34 ` David Airlie 2017-02-21 11:11 ` [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes Thomas Hellstrom 1 sibling, 1 reply; 6+ messages in thread From: Thomas Hellstrom @ 2017-02-21 5:32 UTC (permalink / raw) To: Daniel Vetter Cc: Intel Graphics Development, DRI Development, Daniel Vetter, Dave Airlie, linux-kernel@vger.kernel.org No. IMO Not fixing this immediately through stable is out of the question. The deal is that we don't break userspace. Having said that, I'm not against a long term vmwgfx-only solution. But let's fix this now. Admittedly we missed testing this but you got to understand that not all developer teams have a multitude of developers (we have on average one for the whole linux graphics driver stack except GL), and the bug doesn't show up for QE on regression testing unless they run gnome-sheel/Wayland which they currently don't, and I guess they've been focused on the fb2 regression. It's no secret that we've been using the control nodes for some time. The CONTROL_ALLOW is present in the driver private ioctls and the commit has been there since 2016. The user-space code has been present in vmware-tools also since that commit and due to the long release cycles of open-vm-tools the open-vm-tools version was just about to be released. It's necessary for non-xorg /Thomas On 02/20/2017 11:22 PM, Daniel Vetter wrote: > On Sun, Feb 19, 2017 at 4:21 PM, Thomas Hellstrom <thomas@shipmail.org> wrote: >> So I think we need a quick revert of this commit or a quick stable >> follow-up to unbreak things on our side. > I'd much prefer we just register control nodes for vmwgfx only, with a > commit message explaining in detail what exactly your control tool is > using (i.e. which ioctl), plus links to the source code for future > references. Also not sold on the immediate revert, this stuff has been > merged since months. > -Daniel > >> /Thomas >> >> >> On 02/19/2017 03:54 PM, Thomas Hellstrom wrote: >>> Hi! >>> >>> This patch breaks the vmwgfx resolutionKMS daemon which opens a control >>> node to tell DRM about the monitor layout... >>> >>> /Thomas >>> >>> >>> On 10/28/2016 10:10 AM, Daniel Vetter wrote: >>>> Looking at the ioctl permission checks I noticed that it's impossible >>>> to import gem buffers into a control nodes, and fd2handle/handle2fd >>>> also don't work, so no joy with dma-bufs. >>>> >>>> The only way to do anything with a control node is by drawing stuff >>>> into a dumb buffer and displaying that. I suspect control nodes are an >>>> entirely unused thing, and a cursory check shows that there does not >>>> seem to be any callers of drmOpenControl nor of the other drmOpen >>>> functions using DRM_MODE_CONTROL. >>>> >>>> Since I don't like dead uabi, let's remove it. But since this would be >>>> a really big change I think it's better to start out small by simply >>>> not registering anything. We can garbage-collect the dead code later >>>> on, once we're sure it's really not used anywhere. >>>> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_drv.c | 6 ------ >>>> 1 file changed, 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>> index 6efdba4993fc..f085e28ffc6f 100644 >>>> --- a/drivers/gpu/drm/drm_drv.c >>>> +++ b/drivers/gpu/drm/drm_drv.c >>>> @@ -517,12 +517,6 @@ int drm_dev_init(struct drm_device *dev, >>>> goto err_free; >>>> } >>>> >>>> - if (drm_core_check_feature(dev, DRIVER_MODESET)) { >>>> - ret = drm_minor_alloc(dev, DRM_MINOR_CONTROL); >>>> - if (ret) >>>> - goto err_minors; >>>> - } >>>> - >>>> if (drm_core_check_feature(dev, DRIVER_RENDER)) { >>>> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER); >>>> if (ret) >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) 2017-02-21 5:32 ` DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) Thomas Hellstrom @ 2017-02-21 5:34 ` David Airlie 2017-02-21 10:52 ` Thomas Hellstrom 0 siblings, 1 reply; 6+ messages in thread From: David Airlie @ 2017-02-21 5:34 UTC (permalink / raw) To: Thomas Hellstrom Cc: Daniel Vetter, Intel Graphics Development, DRI Development, Daniel Vetter, linux-kernel > > No. > > IMO Not fixing this immediately through stable is out of the question. > The deal is that we don't break userspace. > Having said that, I'm not against a long term vmwgfx-only solution. But > let's fix this now. > > Admittedly we missed testing this but you got to understand that not all > developer teams have a multitude of > developers (we have on average one for the whole linux graphics driver > stack except GL), and the bug > doesn't show up for QE on regression testing unless they run > gnome-sheel/Wayland which they currently don't, and I guess they've been > focused on the fb2 regression. > > It's no secret that we've been using the control nodes for some time. > The CONTROL_ALLOW is present in the > driver private ioctls and the commit has been there since 2016. > > The user-space code has been present in vmware-tools also since that > commit and due to the long release cycles of > open-vm-tools the open-vm-tools version was just about to be released. > It's necessary for non-xorg can you send a revert against drm-next? I'm not sure how clean it will be. there might be an intermediate step. Then can we port vmtools of this behaviour, not even sure what it is doing. Dave. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) 2017-02-21 5:34 ` David Airlie @ 2017-02-21 10:52 ` Thomas Hellstrom 0 siblings, 0 replies; 6+ messages in thread From: Thomas Hellstrom @ 2017-02-21 10:52 UTC (permalink / raw) To: David Airlie Cc: Daniel Vetter, Intel Graphics Development, DRI Development, Daniel Vetter, linux-kernel On 02/21/2017 06:34 AM, David Airlie wrote: >> No. >> >> IMO Not fixing this immediately through stable is out of the question. >> The deal is that we don't break userspace. >> Having said that, I'm not against a long term vmwgfx-only solution. But >> let's fix this now. >> >> Admittedly we missed testing this but you got to understand that not all >> developer teams have a multitude of >> developers (we have on average one for the whole linux graphics driver >> stack except GL), and the bug >> doesn't show up for QE on regression testing unless they run >> gnome-sheel/Wayland which they currently don't, and I guess they've been >> focused on the fb2 regression. >> >> It's no secret that we've been using the control nodes for some time. >> The CONTROL_ALLOW is present in the >> driver private ioctls and the commit has been there since 2016. >> >> The user-space code has been present in vmware-tools also since that >> commit and due to the long release cycles of >> open-vm-tools the open-vm-tools version was just about to be released. >> It's necessary for non-xorg > can you send a revert against drm-next? I'm not sure how clean it will be. > > there might be an intermediate step. > > Then can we port vmtools of this behaviour, not even sure what it is doing. > > Dave. So after a quick investigation of the impact it looks like the daemon patch was pulled out of the Fedora open-vm-tools update in time. This limits the impact to within VMware where we can update the daemon code and rerun the test cycle. I've posted a patch that makes it possible for us to use render-nodes instead of control nodes. /Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes 2017-02-20 22:22 ` Daniel Vetter 2017-02-21 5:32 ` DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) Thomas Hellstrom @ 2017-02-21 11:11 ` Thomas Hellstrom 1 sibling, 0 replies; 6+ messages in thread From: Thomas Hellstrom @ 2017-02-21 11:11 UTC (permalink / raw) To: Daniel Vetter Cc: Intel Graphics Development, DRI Development, Daniel Vetter, Dave Airlie, linux-kernel@vger.kernel.org On 02/20/2017 11:22 PM, Daniel Vetter wrote: > On Sun, Feb 19, 2017 at 4:21 PM, Thomas Hellstrom <thomas@shipmail.org> wrote: >> So I think we need a quick revert of this commit or a quick stable >> follow-up to unbreak things on our side. > I'd much prefer we just register control nodes for vmwgfx only, with a > commit message explaining in detail what exactly your control tool is > using (i.e. which ioctl), plus links to the source code for future > references. Also not sold on the immediate revert, this stuff has been > merged since months. > -Daniel > https://cgit.freedesktop.org/~thomash/open-vm-tools/commit/?h=feature/thellstrom/resolutionKMS&id=9bf65a22d5a06d3a706bc14578619a56e06f8a24 /Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-21 11:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20161028081050.1042-1-daniel.vetter@ffwll.ch>
[not found] ` <07f5e4a3-f7ef-df5e-d8cc-3759d828719d@shipmail.org>
2017-02-19 15:21 ` [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes Thomas Hellstrom
2017-02-20 22:22 ` Daniel Vetter
2017-02-21 5:32 ` DRM_CONTROL node breakage (Re: [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes) Thomas Hellstrom
2017-02-21 5:34 ` David Airlie
2017-02-21 10:52 ` Thomas Hellstrom
2017-02-21 11:11 ` [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes Thomas Hellstrom
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox