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.
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?
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