Am 14.06.19 um 17:53 schrieb Emil Velikov: > On 2019/06/14, Koenig, Christian wrote: >> Am 14.06.19 um 14:09 schrieb Emil Velikov: >>> On 2019/05/27, Emil Velikov wrote: >>> [SNIP] >>> Hi Christian, >>> >>> >>> In the following, I would like to summarise and emphasize the need for >>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes >>> extra reading it. >>> >>> >>> Today DRM drivers* do not make any distinction between primary and >>> render node clients. >> That is actually not 100% correct. We have a special case where a DRM >> master is allowed to change the priority of render node clients. >> > Can you provide a link? I cannot find that code. See amdgpu_sched_ioctl(). >>> Thus for a render capable driver, any premise of >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy. >> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH >> now is the right direction to take. >> > Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW > ioctls. > > That aside, can you propose an alternative solution that addresses this > and the second point just below? Give me a few days to work on this, it's already Friday 6pm here. Christian. > >>> Considering the examples of flaky or outright missing drmAuth in prime >>> open-source projects (mesa, kmscube, libva) we can reasonably assume >>> other projects exbibit the same problem. >>> >>> For these and/or other reasons, two DRM drivers have dropped DRM_AUTH >>> since day one. >>> >>> Since we are interested in providing consistent and predictable >>> behaviour to user-space, dropping DRM_AUTH seems to be the most >>> effective way forward. >> Well and what do you do with drivers which doesn't implement render nodes? >> > AFAICT there is a single non-DC driver which does not expose - QXL. > I would expect it runs on a rather customised stack. > > Would be great to fix that, but ETIME and it's the only exception to the > rule. > > >>> Of course, I'd be more than happy to hear for any other way to achieve >>> the goal outlined. >>> >>> Based on the series, other maintainers agree with the idea/goal here. >>> Amdgpu not following the same pattern would compromise predictability >>> across drivers and complicate userspace, so I would kindly ask you to >>> reconsider. >>> >>> Alternatively, I see two ways to special case: >>> - New flag annotating amdgpu/radeon - akin to the one proposed earlier >>> - Check for amdgpu/radeon in core DRM >> I perfectly agree that I don't want any special handling for amdgpu/radeon. >> >> My key point is that with DRM_AUTH we created an authorization and >> authentication mess in DRM which is functionality which doesn't belong >> into the DRM subsystem in the first place. >> > Precisely and I've outlined below how to resolve this in the long run. > >>> Now on your pain point - not allowing render iocts via the primary node, >>> and how this patch could make it harder to achieve that goal. >>> >>> While I love the idea, there are number of obstacles that prevent us >>> from doing so at this time: >>> - Ensuring both old and new userspace does not regress >> Yeah, agree totally. That's why I said we should probably start doing >> this for only for upcoming hardware generations. >> > That will side-step the regression issue, but will enforce driver > specific behaviour outlined before. > >>> - Drivers (QXL, others?) do not expose a render node >> Well isn't that is a rather big problem for the removal of DRM_AUTH in >> general? >> >> E.g. at least QXL would then expose functionality on the primary node >> without authentication. >> > With this series QXL remains functionally unchanged. I would love to fix > that as well yet ETIME as mentioned above. > >>> - We want to avoid driver specific behaviour >>> >>> The only way forward that I can see is: >>> - Address QXL/others to expose render nodes >>> - Add a Kconfig toggle to disable !KMS ioctls via the primary node >>> - Jump-start ^^ with distribution X >>> - Fix user-space fallouts >>> - X months down the line, flip the Kconfig >>> - In case of regressions - revert the KConfig, goto Fix user-space... >> Well that at least sounds like a plan :) Let's to this! >> > We're talking about months if not years until it comes to fruition. We > need something quicker. > > That said, I'm quite happy to take the first stab, yet this is not a > replacement for this series. > >>> That said, the proposal will not conflict with the DRM_AUTH removal. If >>> anything it is step 0.5 of the grand master plan. >> That's the point I strongly disagree on. >> >> By lowering the DRM_AUTH restriction you are encouraging userspace to >> use the shortcut and use the primary node for rendering instead of >> fixing the code and using the render node. >> > Have to disagree here. After working on the user-space side and fixing > issues in various projects I can honestly say that most user-space is > sane and developers _care_ about doing things correctly. > >> So at the end of the day userspace will most likely completely drop >> support for the render node, simply for the reason that it became >> superfluous. You can just open up the primary node and get the same >> functionality. >> > I think you're being overly pessimistic here. This is coming from a > person who is often closer to the pessimistic side of things. > > As a middle ground how about we do the following: > - Get this series as-is, alongside > - picking the first three items from the grand master plan > - happy to start a discussion about QXL > - a patch for the Kconfig toggle, is coming in a few minutes > - happy to prod my distribution of choice (Arch) and work with them > > What do you think? > > Thanks > Emil > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
