Skip to content

drm/apple: Support color transformation matrices#119

Closed
jannau wants to merge 91 commits intoAsahiLinux:bits/200-dcpfrom
jannau:bits/200-dcp
Closed

drm/apple: Support color transformation matrices#119
jannau wants to merge 91 commits intoAsahiLinux:bits/200-dcpfrom
jannau:bits/200-dcp

Conversation

@jannau
Copy link
Copy Markdown
Member

@jannau jannau commented Mar 12, 2023

kwin 5.27.3 adds support for "Night Color" via drm "CTM" properties. Wire CTM support up via the "set_matrix" iomfb call.

Link: https://bugs.kde.org/show_bug.cgi?id=455720

alyssarosenzweig and others added 30 commits March 5, 2023 23:15
Add a DRM/KMS driver for Apple system on chips using the DCP
coprocessor, namely the Apple M1. The DCP was added in Apple A14; this
driver does not apply to older iDevices.

This driver targets the DCP firmware API shipped by macOS 12.1.
Currently no incompatibilities with macOS 12.0.1 or 12.2.1 are known.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Co-developed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Xorg startup with modesetting driver triggers this. Move vblank
signalling to dcp to avoid a circular dependency between apple_drv
and dcp.

Signed-off-by: Janne Grunau <j@jannau.net>
Frequency differs between M1 and M1 Pro/Max/Ultra.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
The framebuffer can be unreferenced by GEM while the display controller
is still using it for scanout resulting in IOVA faults and crashed dcp.
dcp has to hold a reference until the swap is complete.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Use the firmware setPowerState callback.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
fixup! WIP: drm/apple: Add DCP display driver

Signed-off-by: Janne Grunau <j@jannau.net>
The swap completes only after the async reply from DCP. Uses
drm_atomic_helper_wait_for_flip_done instead of
drm_atomic_helper_wait_for_vblanks. This should allow ius to get rid
of the scheduled fake vblanks.

Signed-off-by: Janne Grunau <j@jannau.net>
Mostly for the generic callbacks nop, true, false.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Avoids unexpected callbacks on nesting state errors. Encountered when
making DCP calls from a second thread for the backlight.

Signed-off-by: Janne Grunau <j@jannau.net>
Call power-on/-off handling explicitly from apple_crtc_atomic_enable /
apple_crtc_atomic_disable. This makes DPMS work.

Signed-off-by: Janne Grunau <j@jannau.net>
squash! WIP: GPU/apple: t600x work in progress

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Handling it with trampoline_false can result in errors due to
uninitialized data.

Signed-off-by: Janne Grunau <j@jannau.net>
If there is a mismatch in the output size this way we have at leas
consistant results.

Signed-off-by: Janne Grunau <j@jannau.net>
Used by dcp on mode changes on the Macbook Pro 14" (M1 Max).

Signed-off-by: Janne Grunau <j@jannau.net>
This seems to be incorrect but the flag seems to be related to clearing.

Allows unmapping surfaces after the swap finished.

Signed-off-by: Janne Grunau <j@jannau.net>
Use a device_link to ensure piodma has is bound to its DART.

fixup! WIP: drm/apple: Add DCP display driver

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Fixes a display wakeup issue seen with Plasma/X11.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
For external display support DCP will use more endpoints.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
jannau and others added 22 commits March 5, 2023 23:16
Selecting the mode with the highest score may result in HDR mode for
some displays. Since HDR is not support on driver side this produces
an unexpected color representation.
Full parsing allows us to reject virtual color modes (should already be
invalid due to missing "Score").
Preparation for color and timing mode tracing.

Signed-off-by: Janne Grunau <j@jannau.net>
Prevents trace points of unused color modes.

Signed-off-by: Janne Grunau <j@jannau.net>
Restrict symbol mapping for EOTF, pixel encoding and colorimetry to
tracing due to low confidence that it is correct. Main concern is the
colorimetry mapping.

Signed-off-by: Janne Grunau <j@jannau.net>
DCP best scored color mode might result in an HDR mode. As long as the
driver (and DRM) is not ready for HDR try to avoid such modes.

Signed-off-by: Janne Grunau <j@jannau.net>
Probably not important but avoids an unnecessary difference compred to
macOS.

Signed-off-by: Janne Grunau <j@jannau.net>
Seems to work now with 'surface.colorspace = 12'.

Signed-off-by: Janne Grunau <j@jannau.net>
Can be used on devices with camera notch to use the full display height
and thus show the notch.

Signed-off-by: Janne Grunau <j@jannau.net>
This reverts commit d395421.

'w30r' is a wide gammut mode. As long as the display is SDR DCP will end
up displaying picture correctly but on HDR displays like the display in
the Macbook Pro 14"/16" (2021) or external displays with HDR EOTF the
picture has oversaturated / black colors.

The non-wide gammut 10-bit per component pixelformats "l10r" and "R10k"
 are not supported.

Signed-off-by: Janne Grunau <j@jannau.net>
This works on both 8-bit and 10-bit modes without any weirdness, and
gives us the native colorspace without any conversion. Color correction
should probably be handled in software anyway.

However, we need to use surface 1 (at least on t600x), since 0 seems
stuck in bg-sRGB mode for some reason...

Signed-off-by: Hector Martin <marcan@marcan.st>
With "drm/apple: Enable 10-bit mode & set colorspace to native" kernel
log messages are shown in an Apple logo shaped region in the middle of
the display when using BGRA.
The field currently identified as "opaque" is mislabeled and has to be
investigated further.

Signed-off-by: Janne Grunau <j@jannau.net>
- opaque -> is_premultiplied
- swap_enabled BIT(31) seems to be update background with
  dcp_swap.bg_color
- add unused fields is_tearing_allowed, ycbcr_matrix, protection_opts,
  unk_num, unk_denom

Changes: use is_premultiplied only for XRGB8/XBGR8, Update background
only when necessary.

Signed-off-by: Janne Grunau <j@jannau.net>
This does not seem to be as racy as drm_aperture_remove_framebuffers()
and seems to reliably takes over simpledrm's device node.

Signed-off-by: Janne Grunau <j@jannau.net>
This check for the "nomodeset" kernel command line parameter in its
register method.

Signed-off-by: Janne Grunau <j@jannau.net>
drm's documentaion explicitly tells us not to use devm_kzalloc(). drm
device structures might out live the device when they are in use by
userspace while the device vanishes.
There was a report of a race between DRM device registration (and
removal of the simpledrm device) and GDM startup.

The component based device binding ensures that all necessary devices
are bind in the probe method of the last missing component.

Technically the piodma-mapper should be a component of dcp but since it
is only used for its iommu it can be a component of the display
subsystem.

Signed-off-by: Janne Grunau <j@jannau.net>
Avoids "[drm] Cannot find any crtc or sizes" during fbdev initialization
if a display is connected.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Fixes following warning when systemd-backlight restores the backlight
level on boot before a mode is set:

Call trace:
  drm_atomic_helper_crtc_duplicate_state+0x58/0x74
  drm_atomic_get_crtc_state+0x84/0x120
  dcp_set_brightness+0xd8/0x21c [apple_dcp]
  backlight_device_set_brightness+0x78/0x130
  ...

Signed-off-by: Janne Grunau <j@jannau.net>
Backlight drivers are expected to use this instead of accessing
backlight properties.

Signed-off-by: Janne Grunau <j@jannau.net>
kwin 5.27.3 adds support for "Night Color" via drm "CTM" properties.
Wire CTM support up via the "set_matrix" iomfb call.

Link: https://bugs.kde.org/show_bug.cgi?id=455720
Signed-off-by: Janne Grunau <j@jannau.net>
@jannau
Copy link
Copy Markdown
Member Author

jannau commented Mar 24, 2023

Announcing Night Color as supported depends on https://invent.kde.org/plasma/kwin/-/merge_requests/3868 (expected in kwin 5.27.4). Without the change kwin fails to wake the display if there was a color temperature while the display was off.

@aykevl
Copy link
Copy Markdown

aykevl commented Mar 26, 2023

For those interested, kwin 5.27.4 is scheduled to be released on 2023-04-04 (3 weeks after kwin 5.27.3 according to the release schedule).

@jannau
Copy link
Copy Markdown
Member Author

jannau commented Mar 28, 2023

Since it makes little sense to merge this before kwin 5.27.4 is released let's merge it via my ongoing dev work as that avoids conflict resolution for the 12.3 and 13.2 FW version support.

@jannau jannau closed this Mar 28, 2023
svenpeter42 pushed a commit that referenced this pull request Apr 17, 2024
[ Upstream commit 3d75b8a ]

Always flush the per-vCPU async #PF workqueue when a vCPU is clearing its
completion queue, e.g. when a VM and all its vCPUs is being destroyed.
KVM must ensure that none of its workqueue callbacks is running when the
last reference to the KVM _module_ is put.  Gifting a reference to the
associated VM prevents the workqueue callback from dereferencing freed
vCPU/VM memory, but does not prevent the KVM module from being unloaded
before the callback completes.

Drop the misguided VM refcount gifting, as calling kvm_put_kvm() from
async_pf_execute() if kvm_put_kvm() flushes the async #PF workqueue will
result in deadlock.  async_pf_execute() can't return until kvm_put_kvm()
finishes, and kvm_put_kvm() can't return until async_pf_execute() finishes:

 WARNING: CPU: 8 PID: 251 at virt/kvm/kvm_main.c:1435 kvm_put_kvm+0x2d/0x320 [kvm]
 Modules linked in: vhost_net vhost vhost_iotlb tap kvm_intel kvm irqbypass
 CPU: 8 PID: 251 Comm: kworker/8:1 Tainted: G        W          6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
 Workqueue: events async_pf_execute [kvm]
 RIP: 0010:kvm_put_kvm+0x2d/0x320 [kvm]
 Call Trace:
  <TASK>
  async_pf_execute+0x198/0x260 [kvm]
  process_one_work+0x145/0x2d0
  worker_thread+0x27e/0x3a0
  kthread+0xba/0xe0
  ret_from_fork+0x2d/0x50
  ret_from_fork_asm+0x11/0x20
  </TASK>
 ---[ end trace 0000000000000000 ]---
 INFO: task kworker/8:1:251 blocked for more than 120 seconds.
       Tainted: G        W          6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119
 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
 task:kworker/8:1     state:D stack:0     pid:251   ppid:2      flags:0x00004000
 Workqueue: events async_pf_execute [kvm]
 Call Trace:
  <TASK>
  __schedule+0x33f/0xa40
  schedule+0x53/0xc0
  schedule_timeout+0x12a/0x140
  __wait_for_common+0x8d/0x1d0
  __flush_work.isra.0+0x19f/0x2c0
  kvm_clear_async_pf_completion_queue+0x129/0x190 [kvm]
  kvm_arch_destroy_vm+0x78/0x1b0 [kvm]
  kvm_put_kvm+0x1c1/0x320 [kvm]
  async_pf_execute+0x198/0x260 [kvm]
  process_one_work+0x145/0x2d0
  worker_thread+0x27e/0x3a0
  kthread+0xba/0xe0
  ret_from_fork+0x2d/0x50
  ret_from_fork_asm+0x11/0x20
  </TASK>

If kvm_clear_async_pf_completion_queue() actually flushes the workqueue,
then there's no need to gift async_pf_execute() a reference because all
invocations of async_pf_execute() will be forced to complete before the
vCPU and its VM are destroyed/freed.  And that in turn fixes the module
unloading bug as __fput() won't do module_put() on the last vCPU reference
until the vCPU has been freed, e.g. if closing the vCPU file also puts the
last reference to the KVM module.

Note that kvm_check_async_pf_completion() may also take the work item off
the completion queue and so also needs to flush the work queue, as the
work will not be seen by kvm_clear_async_pf_completion_queue().  Waiting
on the workqueue could theoretically delay a vCPU due to waiting for the
work to complete, but that's a very, very small chance, and likely a very
small delay.  kvm_arch_async_page_present_queued() unconditionally makes a
new request, i.e. will effectively delay entering the guest, so the
remaining work is really just:

        trace_kvm_async_pf_completed(addr, cr2_or_gpa);

        __kvm_vcpu_wake_up(vcpu);

        mmput(mm);

and mmput() can't drop the last reference to the page tables if the vCPU is
still alive, i.e. the vCPU won't get stuck tearing down page tables.

Add a helper to do the flushing, specifically to deal with "wakeup all"
work items, as they aren't actually work items, i.e. are never placed in a
workqueue.  Trying to flush a bogus workqueue entry rightly makes
__flush_work() complain (kudos to whoever added that sanity check).

Note, commit 5f6de5c ("KVM: Prevent module exit until all VMs are
freed") *tried* to fix the module refcounting issue by having VMs grab a
reference to the module, but that only made the bug slightly harder to hit
as it gave async_pf_execute() a bit more time to complete before the KVM
module could be unloaded.

Fixes: af585b9 ("KVM: Halt vcpu if page it tries to access is swapped out")
Cc: stable@vger.kernel.org
Cc: David Matlack <dmatlack@google.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Link: https://lore.kernel.org/r/20240110011533.503302-2-seanjc@google.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
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.

5 participants