Regression: drm: Lobotomize set_busid nonsense for !pci drivers (a325725633c2)

[Emil Velikov]

On 30 September 2016 at 10:55, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi all
>
> On 30 September 2016 at 04:09, Laszlo Ersek <lersek at redhat.com> wrote:
>> Hello Daniel,
>>
>> On 06/21/16 14:08, daniel.vetter at ffwll.ch (Daniel Vetter) wrote:
>>> We already have a fallback in place to fill out the unique from
>>> dev->unique, which is set to something reasonable in drm_dev_alloc.
>>>
>>> Which means we only need to have a special set_busid for pci devices,
>>> to be able to care the backwards compat code for drm 1.1 around, which
>>> libdrm still needs.
>>>
>>> While developing and testing this patch things blew up in really
>>> interesting ways, and the code is rather confusing in naming things
>>> between the kernel code, ioctl #defines and libdrm. For the next brave
>>> dragon slayer, document all this madness properly in the userspace
>>> interface section of gpu.tmpl.
>>>
>>> v2: Make drm_dev_set_unique static and update kerneldoc.
>>>
>>> v3: Entire rewrite, plus document what's going on for posterity in the
>>> gpu docbook uapi section.
>>>
>>> v4: Drop accidental amdgpu hunk (Emil).
>>>
>>> v5: Drop accidental omapdrm vblank counter change (Emil).
>>>
>
>> This patch (commit a325725633c2) regresses X.org on QEMU's virtio-vga
>> device. Please see
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1366842
>>
>> complete with a bisection log under
>>
>>   drivers/gpu/drm/virtio/
>>
> Having gone through the xserver code as this patch was written, I'm
> not entrirely sure how this can happen.
>
> Since there is a very nice cleanup series that this patch is build
> upon, I'd suggest trying to understand the issue, rather than a blind
> revert.
>
> Here we want to check the xserver/libdrm codepath before/after the
> kernel patch, in particular the following drmOpen, drmAvailable,
> drmSetInterfaceVersion and drmGetBusid. If the xserver is using open()
> with drmSetInterfaceVersion then that is _not_ something it should be
> doing and I'm against working around it in the kernel (fwiw).
>
Skimming through the xserver code, with the xserver patch/fix the
following comes up - before we would use open(card0) while now we'll
opt for drmOpen alongside the drmSetInterfaceVersion/drmGetBusid.

IMHO the proper way (tm) to fix this is to stop using drmOpen (replace
with open), drmAvailable, drmSetInterfaceVersion (drop these two, they
are legacy UMS code) and drmGetBusid (where needed use the drm*Device
API.

Ideally we'll get platform/usb devices support for drm*Device thus one
will be able to correctly query/enumerate all the available devices,
apply heuristics and open() the {card,render,control} node of the
required device.

On the kernel side I'm haven't looked too closely, although things
should be handled correctly -> drm_dev_set_unique, sets the
unique/busid which is called in virtio via
drm_dev_init/drm_dev_alloc/drm_virtio_init


I'll see if I can find some time to untangle this later on today, if
not I'll look into it tomorrow morning.

-Emil