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

[Dave Airlie]

On Tue, May 8, 2012 at 6:54 PM, Adam Jackson <ajax at nwnk.net> wrote:
> 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.

Well the problem is I've no idea what hotplug on any other OS is going
to look like,
and I really don't want to invent an abstraction without input from
either someone

a) who cares about another OS
b) has time to help me now, not in 6 months.

The thing is we hit drm code in these codepaths and I've no idea if they would
be Linux specific or not, but I suppose I can shift a lot of stuff
into os-support if
necessary.

We'd of course need better naming for BUS_UDEV then?

>> 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

Like the old code is going to be needed on non-Linux OSes for non-PCI devices,
I've no idea how to make that go away without other OS developers
investing in it.
(other than the remove it all and make them suffer).

>> 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.

So the "plan" is to have two sets of xf86Screens, and if a driver
support hotplug (it tells the server via a driverfunc call)
you pass it a flag saying it should add a hotplug screen not a
toplevel protocol screen.

>> +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.

Yeah maybe odev is a good name for the BUS as well.

>> +
>> +    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.

Well it could pick up headless things, but the drivers should deal
with that in their
probe code and fall out. If we ever get to adding render nodes we'd
have to match on
those as well at some point.


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

yeah I'll look at that.
>
> Would prefer if this was:
>
> UDEVSOURCES_CFLAGS = @LIBDRM_CFLAGS@
>
> or whatever the appropriate automake is for per-target.

I've no idea how to do that, will have to investigate.

>>  #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.

Yeah I'm sure this could be a lot nicer, but its something I feel this
code doesn't make
any worse. it just doesn't make it any better, apart from the previous
patch falling out
to make claimed meaningful.

>> +    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?

I don't think the concept of Primary device applies to a USB connected device,
since we generally use it for insane things like int10 and stuff.

>
> 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...

Well fbdev can still bind via the pci routine, and hopefully with claiming fixed
if you specifiy it in the xorg.confg

>
> 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.

I've thought about doing that, but it is too much ripping up of code
that nobody has any idea of how it magically works.

There is no way to enumerate non-PCI video devices without a list of
them, KMS drivers is the only way to make it all work.

I'm leaving the old probing functions intact for non-Linux and binary
drivers for now, but I could be tempted to rip them out
on Linux, if I added PCI graphics device to the udev probing I suppose.

>
> How do we not have a macro for this already.

I asked myself that twice today, will have to make one somewhere.

Thanks,
Dave.