panfrost: Fix FD resource_get_handle

When handle->type is WINSYS_HANDLE_TYPE_FD, the caller wants a file descriptor for the BO backing the resource. We previously had two paths for this:

1. If rsrc->scanout is available, we prime the GEM handle from the KMS device (rsrc->scanout->handle) to a file descriptor via the KMS device.

2. If rsrc->scanout is not available, we prime the GEM handle from the GPU (bo->gem_handle) to a file descriptor via the GPU device.

In both cases, the caller passes in a resource (with BO) and expects out a file descriptor. There are no direct GEM handles in the function signature; the caller doesn't care which GEM handle we prime to get the file descriptor. In principle, both paths produce the same file descriptor for the same BO, since both GEM handles represent the same underlying resource (viewed from different devices).

On grounds of redundancy alone, it makes sense to remove the rsrc->scanout path. Why have a path that only works sometimes, when we have another path that works always?

In fact, the issues with the rsrc->scanout path are deeper. rsrc->scanout is populated by renderonly_create_gpu_import_for_resource, which does the following:

1. Get a file descriptor for the resource by resource_get_handle with WINSYS_HANDLE_TYPE_FD
2. Prime the file descriptor to a GEM handle via the KMS device.

Here comes strike number 2: in order to get a file descriptor via the KMS device, we had to /already/ get a file descriptor via the GPU device. If we go down the KMS device path, we effectively round trip:

GPU handle -> fd -> KMS handle -> fd

There is no good reason to do this; if everything works, the fd is the same in each case. If everything works. If.

The lifetimes of the GPU handle and the KMS handle are not necessarily bound. In principle, a resource can be created with scanout (constructing a KMS handle). Then the KMS view can be destroyed (invalidating the GEM handle for the KMS device), even though the underlying resource is still valid. Notice the GPU handle is still valid; its lifetime is tied to the resource itself. Then a caller can ask for the FD for the resource; as the resource is still valid, this is sensible. Under the scanout path, we try to get the FD by priming the GEM handle on the KMS device... but that GEM handle is no longer valid, causing the PRIME ioctl to fail with ENOENT. On the other hand, if we primed the GPU GEM handle, everything works as expected.

These edge cases are not theoretical; recent versions of Xwayland trigger this ENOENT, causing issue #5758 (closed) on all Panfrost devices. As far as I can tell, no other kmsro driver has this 'special' kmsro path; the only part of resource_get_handle that needs special handling for kmsro is getting a KMS handle.

Let's remove the broken, useless path, fix Xwayland, bring us in line with other drivers, and delete some code.

Thank you for coming to my ted talk.

Closes: #5758 (closed) Fixes: 7da251fc ("panfrost: Check in sources for command stream") Signed-off-by: Alyssa Rosenzweig alyssa@collabora.com Cc: mesa-stable

Cc @daniels @tomeu @emersion

added panfrost label

A-b with @emersion's suggestion, the logic is sound.

In any case, R-b me with the comment above fixed, because the simplification is a good reason alone. Although I have a question about the "fix Xwayland" explanation:

Then the KMS view can be destroyed (invalidating the GEM handle for the KMS device)

How can this happen? Shouldn't this set rsrc->scanout = NULL?

TBH, I don't know. See #5758 (closed) for the background...

(I am suspicious that there are multiple issues at play here, and this is only the surface..)

added 1 commit

• e56b9fd1 - panfrost: Simplify panfrost_resource_get_handle

Compare with previous version

resolved all threads

added 2 commits

• 001173a1 - panfrost: Fix FD resource_get_handle
• 9c25df9d - panfrost: Simplify panfrost_resource_get_handle

Compare with previous version

mentioned in issue #5758 (closed)

FWIW, looks good to me as well.

Reviewed-by: James Jones <jajones@nvidia.com>

I did finally get my rockpi4 booting a new kernel last week, and got as far as fetching Mesa there in my effort to look into this. Still, glad you beat me to it. I wouldn't have known what to make of the KMS path.

I still don't understand what's going wrong with the KMS/FD path, or why your change triggered a failure.

Does it end up trying to do a cross-device import when it hits that path on export? Perhaps that trips something up? What confused me was the intermittence. I seems like it would fail/succeed consistently for a given path.

I'm honestly not sure. I don't usually venture out into WSI.

added 2 commits

• 74e4a501 - panfrost: Fix FD resource_get_handle
• 43b5b452 - panfrost: Simplify panfrost_resource_get_handle

Compare with previous version

Tested-by: Dan Johansen <strit@manjaro.org>

On Pinebook Pro.

Thanks.

assigned to @marge-bot

added 38 commits

• 43b5b452...ded9cb90 - 36 commits from branch mesa:main
• a0ce545e - panfrost: Fix FD resource_get_handle
• b610128d - panfrost: Simplify panfrost_resource_get_handle

Compare with previous version

unassigned @marge-bot

assigned to @marge-bot

mentioned in commit alyssa/mesa@51689a2b

mentioned in commit alyssa/mesa@b5734cc1

added 4 commits

• b610128d...22efeec3 - 2 commits from branch mesa:main
• b5734cc1 - panfrost: Fix FD resource_get_handle
• 51689a2b - panfrost: Simplify panfrost_resource_get_handle

Compare with previous version

merged

mentioned in commit 38c88f36