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
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to