Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DCP: Initial support for overlay blending #304

Open
wants to merge 7 commits into
base: bits/200-dcp
Choose a base branch
from

Conversation

chadmed
Copy link

@chadmed chadmed commented Jun 22, 2024

DCP can blend two surfaces, so we should wire this up.

We currently don't really have a valid use case for this functionality, however this will be required to make full use of zero-copy video decode once AVD is merged. Work is ongoing in userspace to properly make use of hardware overlay planes. Given DCP's provenance, the best example of this would be directly scanning out hardware-decoded video to the primary plane, then overlaying subtitles/media controls on a different plane.

Given that DCP dies if we try to blend more than two surfaces, we cannot expose an overlay plane and a hardware cursor plane, as userspace has been designed to expect for the last 30 years. Even if we could, again owing to its provenance DCP cannot deal with the unique requirements of a cursor plane - it crashes if a framebuffer partially clips outside of screenspace. This can be hacked around and it is possible to expose a mostly-working cursor plane, however Kwin seems to be the only compositor that is architecturally capable of gracefully handling this in the same way macOS does (fall back to software when required and go back to hardware once the driver says it's safe). As a part of bringing up support for overlay planes, Kwin will also be able to use a DRM_PLANE_TYPE_OVERLAY for a hardware cursor, negating the need for any hacks. Kwin will safely use a hardware cursor on our overlay plane, and compositors which can't/won't implement a similar feature will just use it as a traditional DRM_PLANE_TYPE_OVERLAY.

This series only implements support for ARGB/ABGR on the overlay plane. This is really only useful for cursors, media player overlays/graphics, etc. and does not magically make video work. The final piece in the zero-copy video playback puzzle will be supporting the direct scanout of planar YUV video formats. This may not be trivial.

drivers/gpu/drm/apple/apple_drv.c Show resolved Hide resolved
drivers/gpu/drm/apple/apple_drv.c Outdated Show resolved Hide resolved
drivers/gpu/drm/apple/apple_drv.c Outdated Show resolved Hide resolved
@@ -318,16 +382,29 @@ static int apple_probe_per_dcp(struct device *dev,
struct apple_crtc *crtc;
struct apple_connector *connector;
struct apple_encoder *enc;
struct drm_plane *primary;
int ret;
struct drm_plane *planes[DCP_MAX_PLANES];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only need a single plane pointer (since we do not support a cursor plane)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the pointers to set the immutable zpos property in a sane way.

if (IS_ERR(primary))
return PTR_ERR(primary);
/* Set up our other planes */
for (i = 1; i < DCP_MAX_PLANES; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any advantage of offering more than overlay plane if we can blend only 2?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I did it like this instead of hard-coding two planes was to make it easier to extend if/when we figure out how to get all four surfaces to play nice somehow. I can revert back to hard coded planes if you don't think there's value in doing it like this

* Plane order is nondeterministic for this iterator. DCP will
* almost always crash at some point if the z order of planes
* flip-flops around. Make sure we are always blending them
* in the correct order.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems strange and I wouldn't expect that changing the order is the cause of the crash but something else. We of course need a deterministic order for proper display.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a race leaving a dangling pointer somewhere from all the surfaces swapping order, but I definitely did isolate the underlying cause to the fact that they were swapping order. If you leave everything as-is and comment out normalize_zpos it will happen.

drivers/gpu/drm/apple/iomfb_template.c Outdated Show resolved Hide resolved
* having 4 surfaces, we can only blend two. Surface 0 is also
* unusable on some machines, so ignore it.
*/
l = MAX_BLEND_SURFACES - new_state->normalized_zpos;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better approach with the current state of supported planes and formats would be to hard-code l based on plane type

@chadmed chadmed force-pushed the dcp-overlay-no-cursor-hack branch 2 times, most recently from 25cb4ec to db6d1c7 Compare July 1, 2024 08:00
@chadmed
Copy link
Author

chadmed commented Jul 1, 2024

Rebased and addressed feedback

@chadmed
Copy link
Author

chadmed commented Aug 30, 2024

Rebased on top of bits/200-dcp and base for PR changed

The for_each_oldnew_plane_in_state iterator is nondeterministic
in terms of the order of planes. DCP expects surfaces to be
fed to it in the correct order. Relying on the iterator to
lazily increment the index into surf[] means we cannot meet
this expectation. The constant reordering of planes in
the surf[] array seems to cause DCP to crash under certain
circumstances. Cursors will also often be drawn under the main
plane, which is less than ideal.

Populate surf[] in the order everyone expects us to. This fixes
a whole host of odd behaviour when wiring up multiple DRM universal
planes.

Signed-off-by: James Calligeros <[email protected]>
Despite having 4 surfaces, DCP can only blend two of them at once.

Constrain swaps to two surfaces, and warn if userspace somehow
tries to give us more to swap.

Signed-off-by: James Calligeros <[email protected]>
Owing to its origin in mobile devices and the Apple TV, DCP seems to
have been designed under the assumption that no one could possibly want
a rectangle to clip the screen. If a rectangle's bottom-right edge
clips the screen, DCP will instead try to scale the destination rectangle
to the best of its ability... until it can't anymore.

DCP is not tolerant to faults and will crash if the onscreen portion of
the framebuffer ends up smaller than 32x32, or if any dimension ends up
entirely offscreen.

Use apple_plane_atomic_check() to reject requested plane states that
could crash DCP. This is the final piece of the puzzle required to
enable preliminary support for overlay planes on Apple Silicon devices.

Signed-off-by: James Calligeros <[email protected]>
DCP is capable of compositing two surfaces in hardware. This is
important for zero-copy video playback, etc.

Set up an overlay plane so that userspace can do cool things with
it.

Signed-off-by: James Calligeros <[email protected]>
Fix the call to drm_atomic_helper_check_plane_state to use
the correct scaling factors.

Signed-off-by: James Calligeros <[email protected]>
Some userspace may not handle invalid plane checks gracefully
when falling back to a software cursor. This will manifest
as the screen freezing, recoverable by moving the cursor away
from a screen edge. Throw a warning once to let the user know
why this has happened.

Signed-off-by: James Calligeros <[email protected]>
Userspace cannot be trusted to give us a sane zpos value, but given DCP's
requirement that the primary plane always be the bottommost surface, we
can't rely on drm_atomic_normalize_zpos() to do the job for us either.

Make the zpos property immutable, and keep the primary plane at zpos 0.

Signed-off-by: James Calligeros <[email protected]>
@chadmed
Copy link
Author

chadmed commented Oct 14, 2024

kwin now supports using overlay planes for cursors (queued for 6.3, backported to 6.2 in fedora) so we now have a use case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants