On Wed, 17 Apr 2019 at 21:49, Dave Airlie <airl...@gmail.com> wrote: > > On Thu, 18 Apr 2019 at 03:30, Koenig, Christian > <christian.koe...@amd.com> wrote: > > > > Am 17.04.19 um 17:51 schrieb Emil Velikov: > > > Hi guys, > > > > > > On 2019/04/17, Daniel Vetter wrote: > > >> On Wed, Apr 17, 2019 at 02:46:06PM +0200, Christian König wrote: > > >>> Am 17.04.19 um 14:35 schrieb Daniel Vetter: > > >>>> On Wed, Apr 17, 2019 at 12:06:32PM +0000, Koenig, Christian wrote: > > >>>>> Am 17.04.19 um 14:00 schrieb Daniel Vetter: > > >>>>>> On Wed, Apr 17, 2019 at 11:18:35AM +0000, Koenig, Christian wrote: > > >>>>>>> Am 17.04.19 um 13:06 schrieb Daniel Vetter: > > >>>>>>>> On Wed, Apr 17, 2019 at 12:29 PM Koenig, Christian > > >>>>>>>> <christian.koe...@amd.com> wrote: > > >>>>>>>> [SNIP] > > >>>>>>> Well, what you guys did here is a serious no-go. > > >>>>>> Not really understanding your reasons for an unconditional nak on > > >>>>>> all this > > >>>>>> still. > > >>>>> Well, let me summarize: You worked around a problem with libva by > > >>>>> breaking another driver and now proposing a rather ugly and only halve > > >>>>> backed workaround for that driver. > > >>>>> > > >>>>> This is a serious no-go. I've already send out a i915 specific change > > >>>>> to > > >>>>> remove DRM_AUTH from IOCTLs which also have DRM_RENDER_ALLOW and > > >>>>> revert > > >>>>> the offending patch. > > >>>> Oh I'm totally fine with the revert if that's what we go with. But that > > >>>> still leaves the issue that it would make sense to drop DRM_AUTH from > > >>>> DRIVER_RENDER capable drivers (not just for libva, but in general). And > > >>>> there's fundamentally 2 options to do that: > > >>>> > > >>>> - This one here (or anything implementing the same idea), to keep radv > > >>>> working. > > >>>> > > >>>> - We drop DRM_AUTH from all drivers with DRIVER_RENDER except amdgpu. > > >>>> How > > >>>> exactly that's doen doesn't matter, but it leaves amdgpu out in the > > >>>> cold. It also doesn't matter whether we get there with revert + > > >>>> lots of > > >>>> patches, or a special hack for amdgpu, in the end amdgpu would be > > >>>> different. Also note that your patch isn't enough, since it > > >>>> doesn't fix > > >>>> the core ioctls. > > >>>> > > >>>> I think from an overall platform pov, the first is the better option. > > >>>> After all we try really hard to avoid driver special cases for these > > >>>> kinds > > >>>> of things. > > >>> Well, I initially thought that radv is doing something unusual or > > >>> broken, > > >>> but after looking deeper into this that is actually not the case. > > >>> > > >>> What radv does is calling an IOCTL and expecting an error message when > > >>> it is > > >>> not authenticated. > > >>> > > >>> It looks like libdrm_amdgpu has switched to using DRM_IOCTL_GET_CLIENT > > >>> to > > >>> figure out if it is authenticated or not, but I clearly remember that we > > >>> haven't done this from the beginning. > > >> Maybe in the radoen code, or some earlier internal amdgpu libdrm code? > > >> First public commit has all the bits: getauth, GetVersion, then the accel > > >> query. > > >> > > >>> Thinking more about this I don't think that this problem is actually > > >>> amdgpu > > >>> specific. So I wouldn't drop DRM_AUTH from all render node drivers at > > >>> all, > > >>> but only from those who have the specific issue with libva. > > >> libva was the initial motivation, the goal of Emil's patch wasn't to just > > >> duct-tape over libva. That would be easier to fix in libva. The point was > > >> that we should be able to allow this in general. > > >> > > >> And we can, except for the radv issue uncovered here. > > >> > > >> So please don't get 100% hung up on the libva thing, that wasn't really > > >> the goal, just the initial motivation. And I still thinks it makes for > > >> all > > >> drivers. So again we have: > > >> > > >> - radv hack > > >> - make amdgpu behave different from everyone else > > >> - keep tilting windmills about "pls use rendernodes, thx" > > >> > > >> I neither like tilting windmills nor making drivers behave different for > > >> no reaason at all. > > > Allow me to jump-in some emails down the line. > > > > > > First and foremost, sincere apologies for upsetting you Christian. If > > > it's of any consolidation - let me assure you the goal is _not_ to break > > > amdgpu or any other driver. > > > > > > Secondly, I would like to ask you for a list of projects so we can look > > > and > > > investigate properly. So far you've mentioned libdrm_amdgpu - I'll look > > > into > > > it. > > > > That is rather hard to come by, because you would also need to look at > > all previously existing driver stacks. > > > > E.g. with the classic R300 driver for example. > > > > > On the topic of "working around libva" - sadly libva is _not_ the only > > > offender. We also had bugs in mesa and kmscube. > > > > > > Considering those are taken as a prime example of "the right way", it's > > > very > > > likely for the mistakes to be repeated in many other projects. > > > > > > Where the common "fix" seems to be "run as root"... > > > > > > > > > As Daniel pointed out, we could be fighting the windmills or we could > > > have a > > > small, admittedly ugly, workaround for amdgpu. > > > > > > If you don't like that workaround in the driver we could move it to core > > > DRM. > > > > > > In either case, I would like to focus on a pragramic solution which works > > > with > > > both old and new userspace. > > > > Well, I seriously think the original committed solution could cause a > > lot of problems and the issue with radv is only the tip of the iceberg. > > > > I mean we just have to ask our self why have we created render nodes in > > the first place? The obvious alternative was to just removed the > > authentication restrictions on the primary node which would have been > > way less code and maintenance burden. > > > > I need to dig up the mailing list archive, but I strongly think that one > > of the main arguments for this approach was to NOT break existing userspace. > > > > Even taking into account that we now don't need to deal with UMS and > > really really old userspace drivers any more it still feels like a to > > high risk going down that route. > > I'm going to revert the original patch in drm-next now. We've been > getting a bit lax lately on the regressions need to get reverted no > matter what rule, and we are getting close to rc6 and it's Easter, so > I don't want to have to care about this, forget about it, remember it > again, end up at 5.2. > Makes sense, thanks Dave.
Feel free to add the following to the revert: Acked-by: Emil Velikov <emil.veli...@collabora.com> > Personally I'm rather in favour of cleaning up the driver ioctls since > I don't like older drivers suddenly having a new ABI without > consideration of side effects. > Fwiw I think we're overloading the meaning of ABI - this is a functional change. Which admittedly I'll have a loser and harder look that it doesn't break things ;-) Thanks Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
