[PATCH] drm: Provide drm_set_busid() fallback

[PATCH] drm: Provide drm_set_busid() fallback

[Thierry Reding]

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);
-- 
1.9.1

Re: [PATCH] drm: Provide drm_set_busid() fallback

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: Provide drm_set_busid() fallback

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1875 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Provide drm_set_busid() fallback

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: Provide drm_set_busid() fallback

[David Herrmann]

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