[PATCH 2/2] xfree86: use udev to provide device enumeration for kms devices

[Adam Jackson]

On 5/8/12 9:40 AM, Dave Airlie wrote:

> On Linux in order for future hotplug work, we are required to interface
> to udev to detect device creation/removal. In order to try and get
> some earlier testing on this, this patch adds the ability to use
> udev for device enumeration on Linux.

The big thing I don't really like about this patch is how it talks about 
udev so much.  The driver hook should be platformProbe or osProbe or 
something instead, and the fact that it's udev on Linux is just an OS 
detail.

> The probing integrates with the pci probing code and will fallback
> to the pci probe and old school probe functions in turn.

What do we need to do to drop the old probe code?  XXX DUDE

> The flags parameter to the probe function will be used later
> to provide hotplug and gpu screen flags for the driver to behave
> in a different way.

Explain this a bit?  What would a driver need to do differently between 
the hotplug and coldplug cases?  Not that I'm opposed to having a flag 
here, just want to know what you envision it for.

> +int
> +config_udev_probe_kms_outputs(config_udev_kms_probe_proc_ptr probe_callback)

Kind of hate the naming collision here with randr outputs.  Possibly we 
should start saying idev vs odev for input/output devices?  Not a big 
deal I guess.

> +{
> +    struct udev *udev;
> +    struct udev_enumerate *enumerate;
> +    struct udev_list_entry *devices, *device;
> +
> +    udev = udev_monitor_get_udev(udev_monitor);
> +    enumerate = udev_enumerate_new(udev);
> +    if (!enumerate)
> +        return 0;
> +
> +    udev_enumerate_add_match_subsystem(enumerate, "drm");
> +    udev_enumerate_add_match_sysname(enumerate, "card[0-9]*");

I think this means we'll try to pick up everything with a drm node at 
all, which would include headless things, and I'm not totally sure 
that's what we want.

> @@ -22,9 +22,13 @@ if DGA
>   DGASOURCES = xf86DGA.c
>   endif
>
> +if CONFIG_UDEV_KMS
> +UDEVSOURCES = xf86udev.c
> +endif
> +

If we were making this an OS detail, this kinda belongs under 
os-support/ instead.

>   	   -I$(srcdir)/../loader -I$(srcdir)/../parser \
>              -I$(srcdir)/../vbe -I$(srcdir)/../int10 \
>   	   -I$(srcdir)/../vgahw -I$(srcdir)/../dixmods/extmod \
> -	   -I$(srcdir)/../modes -I$(srcdir)/../ramdac
> +	   -I$(srcdir)/../modes -I$(srcdir)/../ramdac @LIBDRM_CFLAGS@

Would prefer if this was:

UDEVSOURCES_CFLAGS = @LIBDRM_CFLAGS@

or whatever the appropriate automake is for per-target.

>   #if (defined(__sparc__) || defined(__sparc))&&  !defined(__OpenBSD__)
>   extern _X_EXPORT Bool sbusSlotClaimed;
>   #endif
> +
> +#if defined(XSERVER_UDEV_KMS)
> +extern _X_EXPORT int udevSlotClaimed;
> +#endif
> +

I really hate all of the SlotClaimed variables.  Feels like there's a 
prettier solution.

> +    case BUS_UDEV:
> +        if (!pEnt->bus.id.udev->pdev)
> +            return FALSE;
> +        return ((pEnt->bus.id.udev->pdev->domain == primaryBus.id.pci->domain) &&
> +                (pEnt->bus.id.udev->pdev->bus == primaryBus.id.pci->bus) &&
> +                (pEnt->bus.id.udev->pdev->dev == primaryBus.id.pci->dev) &&
> +                (pEnt->bus.id.udev->pdev->func == primaryBus.id.pci->func));

No USB support?

> diff --git a/hw/xfree86/common/xf86fbBus.c b/hw/xfree86/common/xf86fbBus.c
> index 1e51623..9c984f6 100644
> --- a/hw/xfree86/common/xf86fbBus.c
> +++ b/hw/xfree86/common/xf86fbBus.c
> @@ -54,6 +54,10 @@ xf86ClaimFbSlot(DriverPtr drvp, int chipset, GDevPtr dev, Bool active)
>       EntityPtr p;
>       int num;
>
> +#ifdef XSERVER_UDEV_KMS
> +    if (udevSlotClaimed)
> +        return -1;
> +#endif
>   #ifdef XSERVER_LIBPCIACCESS
>       if (pciSlotClaimed)
>           return -1;

And this is why I hate them.  This is saying if there are any 
platform-enumerated devices at all, you no longer get to use fbdev. 
Which is inconsistent with...

> diff --git a/hw/xfree86/common/xf86pciBus.c b/hw/xfree86/common/xf86pciBus.c
> index d758260..e5f1388 100644
> --- a/hw/xfree86/common/xf86pciBus.c
> +++ b/hw/xfree86/common/xf86pciBus.c
> @@ -367,7 +367,15 @@ xf86GetPciInfoForEntity(int entityIndex)
>           return NULL;
>
>       p = xf86Entities[entityIndex];
> -    return (p->bus.type == BUS_PCI) ? p->bus.id.pci : NULL;
> +    switch (p->bus.type) {
> +    case BUS_PCI:
> +        return p->bus.id.pci;
> +    case BUS_UDEV:
> +        return p->bus.id.udev->pdev;
> +    default:
> +        break;
> +    }
> +    return NULL;

Where we go out of our way to allow both BUS_PCI and BUS_UDEV.

One idea would be to have udev enumerate both DRM-backed and raw 
PCI/USB/misc video devices, and pass the distinction in the Probe 
function as a flag.  If you did that you wouldn't need to try to keep 
other bus types working _with_ BUS_UDEV.

More appealingly, if you did that you could probably redo the entirety 
of initial configuration to be, er, comprehensible.  I suspect you're 
going to find some assumptions in the existing code that conflict with 
the idea of hotplug anyway.

> +#ifdef CONFIG_UDEV_KMS
> +        if ((p->bus.type == BUS_UDEV) && (p->bus.id.udev->pdev)) {
> +            struct pci_device *ud = p->bus.id.udev->pdev;
> +            if (ud->domain == d->domain &&
> +                ud->bus == d->bus &&
> +                ud->dev == d->dev &&
> +                ud->func == d->func)
> +                return FALSE;
> +        }
> +#endif

How do we not have a macro for this already.

- ajax