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

[Jonathan Gray]

On Thu, Oct 20, 2016 at 11:44:46AM +0100, Emil Velikov wrote:
> 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

Thanks, I'll try find some time to come up with something that works
first and then propose it on dri-devel.