From: Thierry Reding treding@nvidia.com
The only reason why struct drm_bus is still around is because the SETVERSION IOCTL calls a bus specific .set_busid() function. This commit provides a fallback implementation if a device either doesn't have a bus associated with it or if it doesn't implement .set_busid(). The bus ID will be set to the device's name as returned by dev_name().
This can be useful to create DRM devices directly in drivers using the drm_dev_alloc() and drm_dev_register() functions rather than going through the bus-specific implementations, with the goal of eventually getting rid of drm_bus entirely.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/drm_ioctl.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 93a42040bedb..d27134a94d69 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -122,6 +122,19 @@ err: return ret; }
+static int __drm_set_busid(struct drm_device *dev, struct drm_master *master) +{ + const char *name = dev_name(dev->dev); + + master->unique = kstrdup(name, GFP_KERNEL); + if (!master->unique) + return -ENOMEM; + + master->unique_len = strlen(name); + + return 0; +} + static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) { struct drm_master *master = file_priv->master; @@ -130,9 +143,16 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) if (master->unique != NULL) drm_unset_busid(dev, master);
- ret = dev->driver->bus->set_busid(dev, master); - if (ret) - goto err; + if (dev->driver->bus && dev->driver->bus->set_busid) { + ret = dev->driver->bus->set_busid(dev, master); + if (ret) + goto err; + } else { + ret = __drm_set_busid(dev, master); + if (ret) + goto err; + } + return 0; err: drm_unset_busid(dev, master);
On Fri, Apr 11, 2014 at 03:28:56PM +0200, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
The only reason why struct drm_bus is still around is because the SETVERSION IOCTL calls a bus specific .set_busid() function. This commit provides a fallback implementation if a device either doesn't have a bus associated with it or if it doesn't implement .set_busid(). The bus ID will be set to the device's name as returned by dev_name().
This can be useful to create DRM devices directly in drivers using the drm_dev_alloc() and drm_dev_register() functions rather than going through the bus-specific implementations, with the goal of eventually getting rid of drm_bus entirely.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/drm_ioctl.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 93a42040bedb..d27134a94d69 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -122,6 +122,19 @@ err: return ret; }
+static int __drm_set_busid(struct drm_device *dev, struct drm_master *master) +{
- const char *name = dev_name(dev->dev);
- master->unique = kstrdup(name, GFP_KERNEL);
- if (!master->unique)
return -ENOMEM;- master->unique_len = strlen(name);
- return 0;
+}
static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) { struct drm_master *master = file_priv->master; @@ -130,9 +143,16 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) if (master->unique != NULL) drm_unset_busid(dev, master);
- ret = dev->driver->bus->set_busid(dev, master);
- if (ret)
goto err;
- if (dev->driver->bus && dev->driver->bus->set_busid) {
ret = dev->driver->bus->set_busid(dev, master);if (ret)goto err;- } else {
Hm, my plan was actually to just provide a drm_dev_setunique to drivers so that they can set whatever their userspace wants, and then have no set_busid implementation here at all for !pci. Some userspace at least uses the unique thing to match for the driver, so we need to do the usual bending over backwards to keep it consistent.
The approach with a drm_set_unique helper would also make conversion of existing platform and usb drivers easier.
-Daniel
ret = __drm_set_busid(dev, master);if (ret)goto err;- }
- return 0;
err: drm_unset_busid(dev, master); -- 1.9.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 11, 2014 at 07:43:18PM +0200, Daniel Vetter wrote:
On Fri, Apr 11, 2014 at 03:28:56PM +0200, Thierry Reding wrote:
[...]
- ret = dev->driver->bus->set_busid(dev, master);
- if (ret)
goto err;
- if (dev->driver->bus && dev->driver->bus->set_busid) {
ret = dev->driver->bus->set_busid(dev, master);if (ret)goto err;- } else {
Hm, my plan was actually to just provide a drm_dev_setunique to drivers so that they can set whatever their userspace wants,
So that would be going one step further and not call drm_set_busid() in the first place, but rather just let drivers statically assign unique at probe/load time? Yeah, that makes sense. It's not like the unique string is going to change at runtime, is it?
and then have no set_busid implementation here at all for !pci.
Perhaps for PCI there could simply be a common helper to initialize the unique string in the way it's currently generated by the PCI bus' implementation of .set_busid(). Even for PCI it's still a static string that's fixed at probe/load time, isn't it? And it isn't specific per device, only per drm_bus.
Some userspace at least uses the unique thing to match for the driver, so we need to do the usual bending over backwards to keep it consistent.
For new drivers and userspace it should be okay to just match on the driver name. Any differentiation on a per-device basis is probably better done using a GET_PARAM ioctl than by parsing a bus ID string.
Or what is userspace doing with the bus ID in the first place?
The approach with a drm_set_unique helper would also make conversion of existing platform and usb drivers easier.
Yeah, I like that better than what this patch does. Is that something you have already queued up? If not I could take a stab at it.
Thierry
On Fri, Apr 11, 2014 at 8:30 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Fri, Apr 11, 2014 at 07:43:18PM +0200, Daniel Vetter wrote:
On Fri, Apr 11, 2014 at 03:28:56PM +0200, Thierry Reding wrote:
[...]
- ret = dev->driver->bus->set_busid(dev, master);
- if (ret)
goto err;
- if (dev->driver->bus && dev->driver->bus->set_busid) {
ret = dev->driver->bus->set_busid(dev, master);if (ret)goto err;- } else {
Hm, my plan was actually to just provide a drm_dev_setunique to drivers so that they can set whatever their userspace wants,
So that would be going one step further and not call drm_set_busid() in the first place, but rather just let drivers statically assign unique at probe/load time? Yeah, that makes sense. It's not like the unique string is going to change at runtime, is it?
It's static, except for pci. There it can change depending upon the drm version set by userspace.
and then have no set_busid implementation here at all for !pci.
Perhaps for PCI there could simply be a common helper to initialize the unique string in the way it's currently generated by the PCI bus' implementation of .set_busid(). Even for PCI it's still a static string that's fixed at probe/load time, isn't it? And it isn't specific per device, only per drm_bus.
Well if you mean static string = something known at compile time put into the .data section then no. At least for pci it's generated at runtime in 2 different version. I think for everything else it's actually static data (driver name usually).
Some userspace at least uses the unique thing to match for the driver, so we need to do the usual bending over backwards to keep it consistent.
For new drivers and userspace it should be okay to just match on the driver name. Any differentiation on a per-device basis is probably better done using a GET_PARAM ioctl than by parsing a bus ID string.
Or what is userspace doing with the bus ID in the first place?
libdrm expose an drmOpenByBusid, which is used by a lot of places to match pci devices to drm drivers. And mesa/Xorg ... have big pci id tables to match pci devices to userspace drivers. On non-pci it's all different afaik.
The approach with a drm_set_unique helper would also make conversion of existing platform and usb drivers easier.
Yeah, I like that better than what this patch does. Is that something you have already queued up? If not I could take a stab at it.
Nope, not yet done really. -Daniel
Hi
On Fri, Apr 11, 2014 at 7:43 PM, Daniel Vetter daniel@ffwll.ch wrote:
Hm, my plan was actually to just provide a drm_dev_setunique to drivers so that they can set whatever their userspace wants, and then have no set_busid implementation here at all for !pci. Some userspace at least uses the unique thing to match for the driver, so we need to do the usual bending over backwards to keep it consistent.
What's the different between a hard-coded per-driver string and dev_name()? I mean, doesn't the device name include the driver string? Or do drivers require more fine-grained naming? Like chipset-specific suffixes/prefixes in the busid?
Anyway, I'm fine with both and really like the approach of killing of drm_bus. So all acked-by me.
Thanks David
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann -
Thierry Reding