drm: apple: Adaptive Sync/ProMotion#477
drm: apple: Adaptive Sync/ProMotion#477chadmed wants to merge 8 commits intoAsahiLinux:asahi-wipfrom
Conversation
jannau
left a comment
There was a problem hiding this comment.
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.
| struct dcp_color_mode sdr; | ||
| struct dcp_color_mode best; | ||
| bool vrr; | ||
| s64 min_vrr; |
There was a problem hiding this comment.
32 bit should be enough and I assume this should be an unsigned value
There was a problem hiding this comment.
They are expressed in TimingElements as 64-bit signed integers, so it is simpler to just store them the same way.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
These are either Q8.16 or Q16.16 in the bottom 24/32 bits of the field in TimingElements. Given how DCP logs minRR and the refresh rate limitations we have I suspect the former, but I've no way to be totally certain. Looks like the other refresh rate fields are actually the same, so maybe we should extend this to PreciseSyncRate etc. too?
585c039 to
ae20e32
Compare
2e41bc2 to
31bf70d
Compare
|
|
||
| dcp_set_digital_out_mode(dcp, false, &dcp->mode, | ||
| complete_set_digital_out_mode, cookie); | ||
| if (mode->vrr && vrr) |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
I don't see the point in accommodating for this when it is explicitly forbidden and unexpected as of right now. Exposing the connector property while we require a modeset also causes Kwin to get confused. It never actually enables VRR but it says it has, which I can foresee resulting in questions from users. I'd rather just hide all of this from userspace for now and pretend it doesn't exist until a decision has been made upstream.
| struct dcp_color_mode sdr; | ||
| struct dcp_color_mode best; | ||
| bool vrr; | ||
| s64 min_vrr; |
There was a problem hiding this comment.
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)
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>
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>
Tested:
Not tested:
What works:
What doesn't
What needs work