Skip to content

drm: apple: Adaptive Sync/ProMotion#477

Open
chadmed wants to merge 35 commits intoAsahiLinux:asahi-wipfrom
chadmed:dcp/vrr
Open

drm: apple: Adaptive Sync/ProMotion#477
chadmed wants to merge 35 commits intoAsahiLinux:asahi-wipfrom
chadmed:dcp/vrr

Conversation

@chadmed
Copy link
Copy Markdown
Member

@chadmed chadmed commented Apr 5, 2026

Tested:

  • Adaptive Sync over DisplayPort (Dell S2721DGF, [48, 165] Hz)
  • Apple ProMotion on 14" and 16" MacBook Pros (tested [24, 120] Hz)

Not tested:

  • Adaptive Sync over HDMI (I have no HDMI 2.x displays)

What works:

  • "Modesetting" in and out of Adaptive Sync
  • mpv... mostly
  • Games running via Proton via Steam via FEX via muvm
  • Native AArch64 games
  • Forcing Adaptive Sync on and sitting at the desktop at minimum refresh rate

What doesn't

  • Firefox will always render at the display's refresh rate
  • On the internal MBP displays only, mpv's gpu and gpu-next outputs leak file descriptors and halt the GPU when in fullscreen. Other backends work fine, other applications work fine, gpu and gpu-next work fine when not in fullscreen...

What needs work

  • The VRR timestamps are best guesses only. We know they are always within a few frames of each other, meaning that they are likely something to do with swap submission and presentation time, but what exactly remains unclear.
  • Wrapping set_digital_out_mode in a dcpep callback seems kind of silly? There's probably a smarter way to do this.

jannau and others added 27 commits April 3, 2026 18:14
After discussion with the devicetree maintainers we agreed to not extend
lists with the generic compatible "apple,wdt" anymore [1]. Use
"apple,t8103-wdt" as base compatible as it is the SoC the driver and
bindings were written for.

[1]: https://lore.kernel.org/asahi/12ab93b7-1fc2-4ce0-926e-c8141cfe81bf@kernel.org/

Reviewed-by: Neal Gompa <neal@gompa.dev>
Signed-off-by: Janne Grunau <j@jannau.net>
The modules-cpio-pkg target added in commit 2a9c8c0 ("kbuild: add
target to build a cpio containing modules") is incompatible with
initramfs with merged /lib and /usr/lib directories [1]. "/lib" cannot
be a link and directory at the same time.
Respect a non-empty INSTALL_MOD_PATH in the modules-cpio-pkg target so
that `make INSTALL_MOD_PATH=/usr modules-cpio-pkg` results in the same
module install location as `make INSTALL_MOD_PATH=/usr modules_install`.

Tested with Fedora distribution initramfs produced by dracut.

Link: https://systemd.io/THE_CASE_FOR_THE_USR_MERGE/ [1]
Fixes: 2a9c8c0 ("kbuild: add target to build a cpio containing modules")
Cc: stable@vger.kernel.org
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Signed-off-by: Janne Grunau <j@jannau.net>
…en calling drm_dev_unplug"

This reverts commit 6bee098.

Den 2026-03-25 kl. 22:11, skrev Simona Vetter:
> On Wed, Mar 25, 2026 at 10:26:40AM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> On Fri, Mar 13, 2026 at 04:17:27PM +0100, Maarten Lankhorst wrote:
>>> When trying to do a rather aggressive test of igt's "xe_module_load
>>> --r reload" with a full desktop environment and game running I noticed
>>> a few OOPSes when dereferencing freed pointers, related to
>>> framebuffers and property blobs after the compositor exits.
>>>
>>> Solve this by guarding the freeing in drm_file with drm_dev_enter/exit,
>>> and immediately put the references from struct drm_file objects during
>>> drm_dev_unplug().
>>>
>>
>> With this patch in v6.18.20, I get the warning backtraces below.
>> The backtraces are gone with the patch reverted.
>
> Yeah, this needs to be reverted, reasoning below. Maarten, can you please
> take care of that and feed the revert through the usual channels? I don't
> think it's critical enough that we need to fast-track this into drm.git
> directly.
>
> Quoting the patch here again:
>
>>  drivers/gpu/drm/drm_file.c        | 5 ++++-
>>  drivers/gpu/drm/drm_mode_config.c | 9 ++++++---
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index ec82068..f52141f 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -233,6 +233,7 @@ static void drm_events_release(struct drm_file *file_priv)
>>  void drm_file_free(struct drm_file *file)
>>  {
>>  	struct drm_device *dev;
>> +	int idx;
>>
>>  	if (!file)
>>  		return;
>> @@ -249,9 +250,11 @@ void drm_file_free(struct drm_file *file)
>>
>>  	drm_events_release(file);
>>
>> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> +	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
>> +	    drm_dev_enter(dev, &idx)) {
>
> This is misplaced for two reasons:
>
> - Even if we'd want to guarantee that we hold a drm_dev_enter/exit
>   reference during framebuffer teardown, we'd need to do this
>   _consistently over all callsites. Not ad-hoc in just one place that a
>   testcase hits. This also means kerneldoc updates of the relevant hooks
>   and at least a bunch of acks from other driver people to document the
>   consensus.
>
> - More importantly, this is driver responsibilities in general unless we
>   have extremely good reasons to the contrary. Which means this must be
>   placed in xe.
>
>>  		drm_fb_release(file);
>>  		drm_property_destroy_user_blobs(dev, file);
>> +		drm_dev_exit(idx);
>>  	}
>>
>>  	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index 84ae8a2..e349418 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -583,10 +583,13 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>  	 */
>>  	WARN_ON(!list_empty(&dev->mode_config.fb_list));
>>  	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
>> -		struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");
>> +		if (list_empty(&fb->filp_head) || drm_framebuffer_read_refcount(fb) > 1) {
>> +			struct drm_printer p = drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]");
>
> This is also wrong:
>
> - Firstly, it's a completely independent bug, we do not smash two bugfixes
>   into one patch.
>
> - Secondly, it's again a driver bug: drm_mode_cleanup must be called when
>   the last drm_device reference disappears (hence the existence of
>   drmm_mode_config_init), not when the driver gets unbound. The fact that
>   this shows up in a callchain from a devres cleanup means the intel
>   driver gets this wrong (like almost everyone else because historically
>   we didn't know better).
>
>   If we don't follow this rule, then we get races with this code here
>   running concurrently with drm_file fb cleanups, which just does not
>   work. Review pointed that out, but then shrugged it off with a confused
>   explanation:
>
>   https://lore.kernel.org/all/e61e64c796ccfb17ae673331a3df4b877bf42d82.camel@linux.intel.com/
>
>   Yes this also means a lot of the other drm_device teardown that drivers
>   do happens way too early. There is a massive can of worms here of a
>   magnitude that most likely is much, much bigger than what you can
>   backport to stable kernels. Hotunplug is _hard_.

Back to the drawing board, and fixing it in the intel display driver
instead.

Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reported-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
Fixes: 6bee098 ("drm: Fix use-after-free on framebuffers and property blobs when calling drm_dev_unplug")
- WLAN/BT (SMC PMU GPIO AsahiLinux#13) (all devices)
- ASM3142 (SMC PMU GPIO AsahiLinux#14) (j434, iMac with 4 USB-C ports)
- SD card reader (SMC PMU GPIO AsahiLinux#23) (j504, 14-inch MacBook Pro)

Signed-off-by: Janne Grunau <j@jannau.net>
The internal keyboard and trackpad HID on MacBook variants
of the Apple M3 (t8122) SoC are connected through a Apple
-developed protocol called DockChannel and mediated by a
coprocessor known as the Multi-Touch Processor (MTP).

This commit adds the nessecary device tree nodes to the
M3's device tree for internal HID to work. It is disabled
by default, to be enabled only in MacBook board files
where it is tested and confirmed to work.

Co-developed-by: Alyssa Milburn <amilburn@zall.org>
Signed-off-by: Alyssa Milburn <amilburn@zall.org>
Signed-off-by: Michael Reeves <michael.reeves077@gmail.com>
Add mtp device nodes for t8122 (M3) based MacBooks.

Signed-off-by: Michael Reeves <michael.reeves077@gmail.com>
Copy link
Copy Markdown
Member

@jannau jannau left a comment

Choose a reason for hiding this comment

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

How did you verify this does anything at all on MacBook Pro internal displays? I would expect that this does nothing without EDID with adaptive sync data.

((horiz.active == 3024 && vert.active == 1964) ||
(horiz.active == 3456 && vert.active == 2234)))
(horiz.active == 3456 && vert.active == 2234))) {
out->min_vrr = 24 << 16;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought the minimal refresh rate for ProMotion was was 10 Hz

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Only on the iPhones and new iPad Pros apparently. There's scant concrete information available for the MacBooks, but Wikipedia lists 24 Hz and this matches what I've experienced. Setting it any lower results in stuttering.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apple documents 24 Hz as minimum for iPad Pros so it's probably the same for macbooks.

struct dcp_color_mode sdr;
struct dcp_color_mode best;
bool vrr;
s64 min_vrr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

32 bit should be enough and I assume this should be an unsigned value

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They are expressed in TimingElements as 64-bit signed integers, so it is simpler to just store them the same way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please sanitize the values and store them as u32. I'd suggest adding parse_int_checked_u32(struct dcp_parse_ctx *handle, u32 *value, u32 min, u32 max)

Comment thread drivers/gpu/drm/apple/iomfb_template.h
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
chadmed added 2 commits April 6, 2026 14:01
IOMFB exposes a method that allows firmware consumers to change
display behaviour parameters at runtime. One such parameter is
IOMFBParameter_adaptive_sync, which allows DCP to be informed
of the desired minimum refresh rate, media target rate, and
fractional rate.

Add an enum to define the supported parameters, and add
IOMFBPARAM_ADAPTIVE_SYNC to it as a starting point.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
This was actually unnecessary, and having dcp_on_set_parameter as
a dcp_callback_t will introduce some complicated duplication when
enabling VRR. Remove this callback and just set the display handle
on poweron instead.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
@chadmed chadmed force-pushed the dcp/vrr branch 2 times, most recently from 585c039 to ae20e32 Compare April 6, 2026 06:22
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
Comment thread drivers/gpu/drm/apple/iomfb_template.c Outdated
static void dcp_on_set_adaptive_sync(struct apple_dcp *dcp, void *out, void *cookie)
{
dcp_set_digital_out_mode(dcp, false, &dcp->mode,
complete_set_digital_out_mode, cookie);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unfortunately a full modeset. I initially assumed it wouldn't trigger full modeset like when trying to set the current mode without changing the minimal refresh rate.
It is even on macOS a full modeset both with USB-C/DP and HDMI (on M2 Pro) so I fear we can't make it work.
What we could possibly do is add a force_vrr module parameter and always set minRR when setting a VRR mode.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had an old implementation that set mode_changed and forced a full modeset. I will go back to that in the first instance if we can't get minRR to do anything without a modeset, and then add the module parameter to force VRR on at all times, ignoring userspace. I wonder if this has improved at all with newer firmware...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Trying to force a modeset on VRR_ENABLED causes something to lock up, but since this is not expected to work anyway...

* like the actual value does not matter, as long as it is non zero.
* TODO: ascertain with certainty what these timestamps
* are. These names are guesses based on what macOS populates
* them with. These values seem to work well with VRR.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure that these names make sense. Since setting them all to the same value enabled 120 Hz refresh rate on the ProMotion display my guess is that of the timestamps is the time base (could be submit time). The other two could intended/earliest display time and latest display time.

chadmed added 6 commits April 7, 2026 22:14
DCP supports VRR/Adaptive Sync, with its enormous firmware blob
handling the low-level details for us. Display refresh rate is
determined by the swap timing values provided to DCP on each
swap request.

VRR is activated by setting IOMFBadaptive_sync_parameter::minRR
and then requesting a modeset.

Wire up all of the required KMS properties to expose VRR to
userspace, and tell DCP to enable it when supported. This
enables VRR *unconditionally* for supported sinks, which will
be fixed in a future commit.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
DCP requires a "modeset" to trigger the upload of the SDP
to the display. On some monitors, this is instant. On others,
this seems to take as long as a real modeset. Given that in
either case we still blank the display, let's just force a
full modeset when VRR is toggled on or off.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
Setting these timestamps to a dummy value worked fine for enabling
a fixed 120 Hz mode on the MacBook Pros, however doing so causes
Adaptive Sync displays to simply switch between full and minimum
refresh rates. Setting these timestamps based on the swap pacing
seems to fix this, and makes the display's refresh rate match
the incoming swap rate.

Note that the names and values are best-guess only. These seem
to work fine for driving VRR displays, but may still be incorrect.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
Since these machines do not have proper EDID/DisplayID data, we need
to help the driver along a little bit. We know that "ProMotion" displays
can do 24-120 Hz VRR, so let's populate the mode with those values.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
macOS is inconsistent with how it uses DCP timestamps. Some swaps
don't use them at all. We know they are required for VRR display
modes to work properly, so let's just turn them on when we are
connected to a VRR display. This includes the 120 Hz mode on
the 14" and 16" MacBook Pros.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
Given that DCP requires a modeset to activate VRR, and given that
this is explicitly banned by KMS API contract and VESA DisplayPort
specification, hide this experimental support behind a module param.

Interestingly, the HDMI spec does not require a modeset-free VRR
transition. For this reason, it is expected that the KMS API contract
may change in the future, as both Intel and AMD hardware require
a modeset to enable VRR in some circumstances. Either VRR will be
expected to be enabled whenever it is supported, *or* modesetting
to toggle it on or off will be allowed. When that happens, this
commit *must* be reverted.

Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
}

if (dcp->vrr_enabled != crtc_state->vrr_enabled) {
dcp->vrr_enabled = crtc_state->vrr_enabled;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dcp->vrr_enabled should toggled in iomfb_modeset()

cookie);
} else {
if (!dcp->main_display)
handle = 2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

handle = dcp->main_display ? 0 : 2;

module_param(unstable_edid, bool, 0644);
MODULE_PARM_DESC(unstable_edid, "Enable unstable EDID retrival support");

bool vrr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

force_vrr


bool vrr;
module_param(vrr, bool, 0644);
MODULE_PARM_DESC(vrr, "Enable Adaptive Sync/ProMotion on supported displays");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Always enable ...


dcp_set_digital_out_mode(dcp, false, &dcp->mode,
complete_set_digital_out_mode, cookie);
if (mode->vrr && vrr)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should also work if compositor allows modesets with enabling vrr without the module parameter set. I expected here something like

if (!force_vrr && dcp->vrr_enabled != crtc_state->vrr_enabled) {
    dcp->vrr_enabled = crtc_state->vrr_enabled;
}

if (mode->vrr) {
    if (force_vrr || dcp->vrr_enabled)
        dcp_set_adaptive_sync(dcp, mode->min_vrr, cookie);
    else
        dcp_set_adaptive_sync(dcp, 0, cookie);
} else {
    		dcp_set_digital_out_mode(dcp, false, &dcp->mode,
					 complete_set_digital_out_mode, cookie);
}

struct dcp_color_mode sdr;
struct dcp_color_mode best;
bool vrr;
s64 min_vrr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please sanitize the values and store them as u32. I'd suggest adding parse_int_checked_u32(struct dcp_parse_ctx *handle, u32 *value, u32 min, u32 max)

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.

3 participants