| Message ID | 20161209135656.14881-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9 December 2016 at 13:56, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We thought that no userspace is using them, but oops libdrm is using > them to figure out whether a driver supports modesetting. Check out > drmCheckModesettingSupported but maybe don't because it's horrible and > totally runs counter to where we want to go with libdrm device > handling. Sincere apologies for missing that one. Must have been looking at one of the branches where I've neutered it. Fwiw, the patch is Acked-by: Emil Velikov <emil.l.velikov@gmail.com> Emil
Hey On Fri, Dec 9, 2016 at 2:56 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We thought that no userspace is using them, but oops libdrm is using > them to figure out whether a driver supports modesetting. Check out > drmCheckModesettingSupported but maybe don't because it's horrible and > totally runs counter to where we want to go with libdrm device > handling. The function looks in the device hierarchy for whether > controlD* exist using the following format string: > > /sys/bus/pci/devices/%04x:%02x:%02x.%d/drm/controlD%d > > The "/drm" subdirectory is the glue directory from the sysfs class > stuff, and the only way to get at it seems to through > kdev->kobj.parent (when kdev is represents e.g. the card0 chardev > instance in sysfs). Git grep says we're not the only ones touching > that, so I hope it's ok we dig into such internals - I couldn't find a > proper interface for getting at the glue directory. > > Quick git grep shows that at least -amdgpu, -ati are using this. > -modesetting do not, and on -intel it's only about the 4th fallback > path for device lookup, which is why this didn't blow up earlier. > > Oh well, we need to keep it working, and the simplest way is to add a > symlink at the right place in sysfs from controlD* to card*. > > v2: > - Fix error path handling by adding if (!minor) return checks (David) > - Fix the controlD* numbers to match what's been there (David) > - Add a comment what exactly userspace minimally needs. > - Correct the analysis for -intel (Chris). > > Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes") > Cc: Dave Airlie <airlied@gmail.com> > Reported-by: Alex Deucher <alexander.deucher@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_drv.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 4ec61ac27477..4a7b3e98d586 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -650,6 +650,62 @@ void drm_dev_unref(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_dev_unref); > > +static int create_compat_control_link(struct drm_device *dev) > +{ > + struct drm_minor *minor; > + char *name; > + int ret; > + > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return 0; > + > + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY); > + if (!minor) > + return 0; > + > + /* > + * Some existing userspace out there uses the existing of the controlD* > + * sysfs files to figure out whether it's a modeset driver. It only does > + * readdir, hence a symlink is sufficient (and the least confusing > + * option). Otherwise controlD* is entirely unused. > + * > + * Old controlD chardev have been allocated in the range > + * 64-127. > + */ > + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index + 64); > + if (!name) > + return -ENOMEM; > + > + ret = sysfs_create_link(minor->kdev->kobj.parent, > + &minor->kdev->kobj, > + name); > + > + kfree(name); > + > + return ret; > +} > + > +static void remove_compat_control_link(struct drm_device *dev) > +{ > + struct drm_minor *minor; > + char *name; > + > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return; > + > + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY); > + if (!minor) > + return; > + > + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index); > + if (!name) > + return; "%d" is at most 11 characters: char name[8 + 11 + 1]; snprintf(name, sizeof(name), "controlD%d", minor->index); sysfs_{create,remove}_link(...); Makes the code in both paths a lot simpler, at the cost of doing buffer-length calculations. Also, it avoids failure in the cleanup path (even though kasprintf() failure is impossible for small buffers). I prefer anything that is not asprintf(). Up to you. Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > + > + sysfs_remove_link(minor->kdev->kobj.parent, name); > + > + kfree(name); > +} > + > /** > * drm_dev_register - Register DRM device > * @dev: Device to register > @@ -688,6 +744,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) > if (ret) > goto err_minors; > > + ret = create_compat_control_link(dev); > + if (ret) > + goto err_minors; > + > if (dev->driver->load) { > ret = dev->driver->load(dev, flags); > if (ret) > @@ -701,6 +761,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) > goto out_unlock; > > err_minors: > + remove_compat_control_link(dev); > drm_minor_unregister(dev, DRM_MINOR_PRIMARY); > drm_minor_unregister(dev, DRM_MINOR_RENDER); > drm_minor_unregister(dev, DRM_MINOR_CONTROL); > @@ -741,6 +802,7 @@ void drm_dev_unregister(struct drm_device *dev) > list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) > drm_legacy_rmmap(dev, r_list->map); > > + remove_compat_control_link(dev); > drm_minor_unregister(dev, DRM_MINOR_PRIMARY); > drm_minor_unregister(dev, DRM_MINOR_RENDER); > drm_minor_unregister(dev, DRM_MINOR_CONTROL); > -- > 2.10.2 >
On Fri, Dec 09, 2016 at 02:56:56PM +0100, Daniel Vetter wrote: > We thought that no userspace is using them, but oops libdrm is using > them to figure out whether a driver supports modesetting. Check out > drmCheckModesettingSupported but maybe don't because it's horrible and > totally runs counter to where we want to go with libdrm device > handling. The function looks in the device hierarchy for whether > controlD* exist using the following format string: > > /sys/bus/pci/devices/%04x:%02x:%02x.%d/drm/controlD%d > > The "/drm" subdirectory is the glue directory from the sysfs class > stuff, and the only way to get at it seems to through > kdev->kobj.parent (when kdev is represents e.g. the card0 chardev > instance in sysfs). Git grep says we're not the only ones touching > that, so I hope it's ok we dig into such internals - I couldn't find a > proper interface for getting at the glue directory. > > Quick git grep shows that at least -amdgpu, -ati are using this. > -modesetting do not, and on -intel it's only about the 4th fallback > path for device lookup, which is why this didn't blow up earlier. > > Oh well, we need to keep it working, and the simplest way is to add a > symlink at the right place in sysfs from controlD* to card*. > > v2: > - Fix error path handling by adding if (!minor) return checks (David) > - Fix the controlD* numbers to match what's been there (David) > - Add a comment what exactly userspace minimally needs. > - Correct the analysis for -intel (Chris). > > Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes") > Cc: Dave Airlie <airlied@gmail.com> > Reported-by: Alex Deucher <alexander.deucher@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 4ec61ac27477..4a7b3e98d586 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -650,6 +650,62 @@ void drm_dev_unref(struct drm_device *dev) } EXPORT_SYMBOL(drm_dev_unref); +static int create_compat_control_link(struct drm_device *dev) +{ + struct drm_minor *minor; + char *name; + int ret; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return 0; + + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY); + if (!minor) + return 0; + + /* + * Some existing userspace out there uses the existing of the controlD* + * sysfs files to figure out whether it's a modeset driver. It only does + * readdir, hence a symlink is sufficient (and the least confusing + * option). Otherwise controlD* is entirely unused. + * + * Old controlD chardev have been allocated in the range + * 64-127. + */ + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index + 64); + if (!name) + return -ENOMEM; + + ret = sysfs_create_link(minor->kdev->kobj.parent, + &minor->kdev->kobj, + name); + + kfree(name); + + return ret; +} + +static void remove_compat_control_link(struct drm_device *dev) +{ + struct drm_minor *minor; + char *name; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return; + + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY); + if (!minor) + return; + + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index); + if (!name) + return; + + sysfs_remove_link(minor->kdev->kobj.parent, name); + + kfree(name); +} + /** * drm_dev_register - Register DRM device * @dev: Device to register @@ -688,6 +744,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors; + ret = create_compat_control_link(dev); + if (ret) + goto err_minors; + if (dev->driver->load) { ret = dev->driver->load(dev, flags); if (ret) @@ -701,6 +761,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto out_unlock; err_minors: + remove_compat_control_link(dev); drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); @@ -741,6 +802,7 @@ void drm_dev_unregister(struct drm_device *dev) list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) drm_legacy_rmmap(dev, r_list->map); + remove_compat_control_link(dev); drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL);
We thought that no userspace is using them, but oops libdrm is using them to figure out whether a driver supports modesetting. Check out drmCheckModesettingSupported but maybe don't because it's horrible and totally runs counter to where we want to go with libdrm device handling. The function looks in the device hierarchy for whether controlD* exist using the following format string: /sys/bus/pci/devices/%04x:%02x:%02x.%d/drm/controlD%d The "/drm" subdirectory is the glue directory from the sysfs class stuff, and the only way to get at it seems to through kdev->kobj.parent (when kdev is represents e.g. the card0 chardev instance in sysfs). Git grep says we're not the only ones touching that, so I hope it's ok we dig into such internals - I couldn't find a proper interface for getting at the glue directory. Quick git grep shows that at least -amdgpu, -ati are using this. -modesetting do not, and on -intel it's only about the 4th fallback path for device lookup, which is why this didn't blow up earlier. Oh well, we need to keep it working, and the simplest way is to add a symlink at the right place in sysfs from controlD* to card*. v2: - Fix error path handling by adding if (!minor) return checks (David) - Fix the controlD* numbers to match what's been there (David) - Add a comment what exactly userspace minimally needs. - Correct the analysis for -intel (Chris). Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes") Cc: Dave Airlie <airlied@gmail.com> Reported-by: Alex Deucher <alexander.deucher@amd.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: David Herrmann <dh.herrmann@gmail.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_drv.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)