[Mesa-dev] [PATCH 02/16] loader: slim down loader_get_pci_id_for_fd implementation(s)

[Emil Velikov]

On 20 October 2016 at 05:35, Jonathan Gray <jsg at jsg.id.au> wrote:
> On Sat, Oct 15, 2016 at 01:32:02PM +0100, Emil Velikov wrote:
>> On Saturday, 15 October 2016, Jonathan Gray <jsg at jsg.id.au> wrote:
>>
>> > On Tue, Oct 11, 2016 at 07:31:46PM +0100, Emil Velikov wrote:
>> > > From: Emil Velikov <emil.velikov at collabora.com <javascript:;>>
>> > >
>> > > Currently mesa has three code paths in the loader - libudev, manual
>> > > sysfs and drm ioctl one.
>> > >
>> > > Considering the issues we had with libudev - strip those down in favour
>> > > of the libdrm drm device API. The latter can be implemented in any way
>> > > depending on the platform and can be reused by others.
>> > >
>> > > Cc: Jonathan Gray <jsg at jsg.id.au <javascript:;>>
>> > > Cc: Jean-S??bastien P??dron <dumbbell at FreeBSD.org>
>> > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com <javascript:;>>
>> > > ---
>> > > Jonathan, Jean-S??bastien I believe I've prodded you guys for a *BSD
>> > > implementation of the drm*Device API a while back. With this commit
>> > > you'll be 'forced' to prep some ;-)
>> >
>> > It has been a while since I looked into that.  The design seemed to
>> > assume that the user running code that called into libdrm had the
>> > ability to enumerate pci busses.
>> >
>> > On OpenBSD /dev/pci* is only read/writable by root.  /dev/drm* is
>> > chowned after a user logs into a console.
>> >
>> > Yes that's correct. The principle is that the platform/kernel has a method
>> of enumerating and providing basic information for the available devices.
>> Note that there are multiple applications explicitly dropping OpenBSD
>> support from their TODO because it lacks the ability [from unprivileged
>> context].
>>
>> We don't use filesystems for communicating with the kernel like linux
>> > does so ioctls are really the best fit.  The loader parts used at the
>> > moment use drm driver specific ioctls, hopefully a generic version of
>> > those that can return the vid/pid, subids and function id numbers would
>> > cover most of it.
>>
>> Afaict retrieving the vendor/device ID et al is not a security concern
>> (admittedly I'm not a security person) nor drm specific.
>>
>> As-is one will end up with multiple implementations - one per subsystem
>> leading to extra code and maintenance burden on both OpenBSD end and for
>> apps who want to support the platform.
>>
>> Hope that makes things clear and doesn't sound too ranty ;-)
>> Emil
>
> The drm functions in question ask for pci information by passing in
> major and minor numbers of drm devices or in the case of
> drmParsePciDeviceInfo bizzarely just "drm0", not sure why the
> inconsistency there.  They aren't enumerating buses they are responding
> based on information from drm sources.  So it's going to be drm not pci
> ioctls.
>
One could rework the lot via a) enumerate {PCI,other} devices first,
b) retrieve bus/device info (vendor/device/nodes) and feed that back
to the user. In practise the end result should not matter.
But I have to agree - the current implementation is not that pretty.
Thus I won't object if one has patches to improve/rewrite it :-)

Wrt the rest, I'm not sure I fully parse what you're saying. It sounds
like you've noticed multiple deficiencies, so it'll be better to keep
that a separate thread

> The real problem is going to be in adding drm ioctls I'd have to convince
> the linux kernel people to reserve numbers, people involved in libdrm
> and people involved in OpenBSD.  Though the functions would then be
> useable for systems beyond just those with sysfs if they implemented the
> ioctls.
>
Chances are you'll get a Nack straight away from the Linux kernel
devs. The idea being that a) there's an already existing userspace
interfaces to do all this stuff and b) the linux kernel ABI must stay
forever, so people are more cautious what gets added.

Regardless, please forward what you have in mind to dri-devel, in case
situation/opinions have changed or I'm misunderstood things.

> I had diffs from a few months ago to do drmGetMinorNameForFD based on
> loader drm_get_device_name_for_fd, drmParsePciDeviceInfo based on
> drm_get_pci_id_for_fd and defaulting to DRM_BUS_PCI rather than an error
> in drmParseSubsystemType.

>  I don't think there is a way to retrieve the
> drmParsePciBusInfo data with the existing ioctls though.
No there isn't any afaict.

In general feel free to approach the whole thing from a different
angle - if it makes more sense/is easier/better wrt OpenBSD.

Thanks
Emil