P010 is a planar 4:2:0 YUV with interleaved UV plane, 10 bits per
channel video format.
P012 is a planar 4:2:0 YUV 12 bits per channel
P016 is a planar 4:2:0 YUV with interleaved UV plane, 16 bits per
channel video format.
V3: Added P012 and fixed cpp for P010.
V4: format definition refined per review.
V5: Format comment block for each new pixel format.
V6: reversed Cb/Cr order in comments.
v7: reversed Cb/Cr order in comments of header files, remove
the wrong part of commit message.
V8: reversed V7 changes except commit message and rebased.
v9: used the new properties to describe those format and
rebased.
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Randy Li <ayaka@soulik.info>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Ayan Kumar Halder <ayan.halder@arm.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190109195710.28501-2-ayaka@soulik.info
Since
commit 39b50c6038 ("drm/atomic_helper: Stop modesets on unregistered
connectors harder")
We've been failing atomic checks if they try to enable new displays on
unregistered connectors. This is fine except for the one situation that
breaks atomic assumptions: suspend/resume. If a connector is
unregistered before we attempt to restore the atomic state, something we
end up failing the atomic check that happens when trying to restore the
state during resume.
Normally this would be OK: we try our best to make sure that the atomic
state pre-suspend can be restored post-suspend, but failures at that
point usually don't cause problems. That is of course, until we
introduced the new atomic MST VCPI helpers:
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:65:pipe B] active changed
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:123:DP-5]
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Disabling [CONNECTOR:123:DP-5]
[drm:drm_atomic_get_private_obj_state [drm]] Added new private object 0000000025844636 state 000000009fd2899a to 000000003a13d7b8
WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper]
Modules linked in: fuse vfat fat snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic joydev iTCO_wdt i915(O) wmi_bmof intel_rapl btusb btrtl x86_pkg_temp_thermal btbcm btintel coretemp i2c_algo_bit drm_kms_helper(O) crc32_pclmul snd_hda_intel syscopyarea sysfillrect snd_hda_codec sysimgblt snd_hda_core bluetooth fb_sys_fops snd_pcm pcspkr drm(O) psmouse snd_timer mei_me ecdh_generic i2c_i801 mei i2c_core ucsi_acpi typec_ucsi typec wmi thinkpad_acpi ledtrig_audio snd soundcore tpm_tis rfkill tpm_tis_core video tpm acpi_pad pcc_cpufreq uas usb_storage crc32c_intel nvme serio_raw xhci_pci nvme_core xhci_hcd
CPU: 6 PID: 1070 Comm: gnome-shell Tainted: G W O 5.0.0-rc2Lyude-Test+ #1
Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018
RIP: 0010:drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper]
Code: 00 4c 39 6d f0 74 49 48 8d 7b 10 48 89 f9 48 c1 e9 03 42 80 3c 21 00 0f 85 d2 00 00 00 48 8b 6b 10 48 8d 5d f0 49 39 ee 75 c5 <0f> 0b 48 c7 c7 c0 78 b3 a0 48 89 c2 4c 89 ee e8 03 6c aa ff b8 ea
RSP: 0018:ffff88841235f268 EFLAGS: 00010246
RAX: ffff88841bf12ab0 RBX: ffff88841bf12aa8 RCX: 1ffff110837e2557
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: ffffed108246bde0
RBP: ffff88841bf12ab8 R08: ffffed1083db3c93 R09: ffffed1083db3c92
R10: ffffed1083db3c92 R11: ffff88841ed9e497 R12: ffff888419555d80
R13: ffff8883bc499100 R14: ffff88841bf12ab8 R15: 0000000000000000
FS: 00007f16fbd4cd00(0000) GS:ffff88841ed80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f1687c9f000 CR3: 00000003ba3cc003 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
drm_atomic_helper_check_modeset+0xf21/0x2f50 [drm_kms_helper]
? drm_atomic_helper_commit_modeset_enables+0xa90/0xa90 [drm_kms_helper]
? __printk_safe_exit+0x10/0x10
? save_stack+0x8c/0xb0
? vprintk_func+0x96/0x1bf
? __printk_safe_exit+0x10/0x10
intel_atomic_check+0x234/0x4750 [i915]
? printk+0x9f/0xc5
? kmsg_dump_rewind_nolock+0xd9/0xd9
? _raw_spin_lock_irqsave+0xa4/0x140
? drm_atomic_check_only+0xb1/0x28b0 [drm]
? drm_dbg+0x186/0x1b0 [drm]
? drm_dev_dbg+0x200/0x200 [drm]
? intel_link_compute_m_n+0xb0/0xb0 [i915]
? drm_mode_put_tile_group+0x20/0x20 [drm]
? skl_plane_format_mod_supported+0x17f/0x1b0 [i915]
? drm_plane_check_pixel_format+0x14a/0x310 [drm]
drm_atomic_check_only+0x13c4/0x28b0 [drm]
? drm_state_info+0x220/0x220 [drm]
? drm_atomic_helper_disable_plane+0x1d0/0x1d0 [drm_kms_helper]
? pick_single_encoder_for_connector+0xe0/0xe0 [drm_kms_helper]
? kasan_unpoison_shadow+0x35/0x40
drm_atomic_commit+0x3b/0x100 [drm]
drm_atomic_helper_set_config+0xd5/0x100 [drm_kms_helper]
drm_mode_setcrtc+0x636/0x1660 [drm]
? vprintk_func+0x96/0x1bf
? drm_dev_dbg+0x200/0x200 [drm]
? drm_mode_getcrtc+0x790/0x790 [drm]
? printk+0x9f/0xc5
? mutex_unlock+0x1d/0x40
? drm_mode_addfb2+0x2e9/0x3a0 [drm]
? rcu_sync_dtor+0x2e0/0x2e0
? drm_dbg+0x186/0x1b0 [drm]
? set_page_dirty+0x271/0x4d0
drm_ioctl_kernel+0x203/0x290 [drm]
? drm_mode_getcrtc+0x790/0x790 [drm]
? drm_setversion+0x7f0/0x7f0 [drm]
? __switch_to_asm+0x34/0x70
? __switch_to_asm+0x34/0x70
drm_ioctl+0x445/0x950 [drm]
? drm_mode_getcrtc+0x790/0x790 [drm]
? drm_getunique+0x220/0x220 [drm]
? expand_files.part.10+0x920/0x920
do_vfs_ioctl+0x1a1/0x13d0
? ioctl_preallocate+0x2b0/0x2b0
? __fget_light+0x2d6/0x390
? schedule+0xd7/0x2e0
? fget_raw+0x10/0x10
? apic_timer_interrupt+0xa/0x20
? apic_timer_interrupt+0xa/0x20
? rcu_cleanup_dead_rnp+0x2c0/0x2c0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x6f/0xb0
do_syscall_64+0x136/0x440
? syscall_return_slowpath+0x2d0/0x2d0
? do_page_fault+0x89/0x330
? __do_page_fault+0x9c0/0x9c0
? prepare_exit_to_usermode+0x188/0x200
? perf_trace_sys_enter+0x1090/0x1090
? __x64_sys_sigaltstack+0x280/0x280
? __put_user_4+0x1c/0x30
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f16ff89a09b
Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007fff001232b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007fff001232f0 RCX: 00007f16ff89a09b
RDX: 00007fff001232f0 RSI: 00000000c06864a2 RDI: 000000000000000b
RBP: 00007fff001232f0 R08: 0000000000000000 R09: 000055a79d484460
R10: 000055a79d44e770 R11: 0000000000000246 R12: 00000000c06864a2
R13: 000000000000000b R14: 0000000000000000 R15: 000055a79d44e770
WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper]
---[ end trace d536c05c13c83be2 ]---
[drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000f9e2b143] found in mst state 000000009fd2899a
This appears to be happening because we destroy the VCPI allocations
when disabling all connected displays while suspending, and those VCPI
allocations don't get restored on resume due to failing to restore the
atomic state.
So, fix this by introducing the suspending option to
drm_atomic_helper_duplicate_state() and use that to indicate in the
atomic state that it's being used for suspending or resuming the system,
and thus needs to be fixed up by the driver. We can then use the new
state->duplicated hook to tell update_connector_routing() in
drm_atomic_check_modeset() to allow for modesets on unregistered
connectors, which allows us to restore atomic states that contain MST
topologies that were removed after the state was duplicated and thus:
mostly fixing suspend and resume. This just leaves some issues that were
introduced with nouveau, that will be addressed next.
Changes since v3:
* Remove ->duplicated hunks that I left in the VCPI helpers by accident.
These don't need to be here, that was the supposed to be the purpose
of the last revision
Changes since v2:
* Remove the changes in this patch to the VCPI helpers, they aren't
needed anymore
Changes since v1:
* Rename suspend_or_resume to duplicated
Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: eceae14724 ("drm/dp_mst: Start tracking per-port VCPI allocations")
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190202002023.29665-4-lyude@redhat.com
This is only used by drm_irq_install(), which is an optional helper.
For legacy pci devices this is required (due to interrupt sharing without
msi/msi-x), and just making this the default exactly matches the behaviour
of all existing drivers using the drm_irq_install() helpers. In case that
ever becomes wrong drivers can roll their own irq handling, as many
drivers already do (for other reasons like needing a threaded interrupt
handler, or having an entire pile of different interrupt sources).
v2: Rebase
v3: Improve commit message (Emil)
Cc: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190129104248.26607-3-daniel.vetter@ffwll.ch
- Unwind failure on pinning the gen7 PPGTT (Chris)
- Fastset updates to make sure DRRS and PSR are properly enabled (Hans)
- Header include clean-up (Brajeswar, Jani)
- Improvements and clean-up on debugfs (Chris, Jani)
- Avoid division by zero on CNL clocks setup (Xiao)
- Restrict PSMI context load w/a to Haswell GT1 (Chris)
- Remove HW semaphores for gen7 inter-engine sync (Chris)
- Pull the render flush into breadcrumb emission (Chris)
- i915_params copy and free helpers and other reorgs and docs (Jani)
- Remove has_pooled_eu static initializer (Tvrtko)
- Updates on kerneldoc (Chris)
- Remove redundant trailing request flush (Chris)
- ringbuffer irq seqno fixes and clean-up (Chris)
- splitting off runtime device info and other clean-up around (Jani)
- Selftests improvements (Chris, Daniele)
- Flush RING_IMR changes before changing the global GT IMR on gen6 and HSW (Chris)
- Some improvements and fixes around GPU reset and GPU hang report (Chris)
- Remove partial attempt to swizzle on pread/pwrite (Chris)
- Return immediately if trylock fails for direct-reclaim (Chris)
- Downgrade scare message for unknown HuC firmware (Jani)
- ACPI / PMIC for MIPI / DSI (Hans)
- Reduce i915_request_alloc retirement to local context (Chris)
- Init per-engine WAs for all engines (Daniele)
- drop DPF code for gen8+ (Daniele)
- Guard error capture against unpinned vma (Chris)
- Use mutex_lock_killable from inside the shrinker (Chris)
- Removing pooling from struct_mutex from vmap shrinker (Chris)
Signed-off-by: Dave Airlie <airlied@redhat.com>
# gpg: Signature made Fri 11 Jan 2019 09:58:18 AEST
# gpg: using RSA key FA625F640EEB13CA
# gpg: Good signature from "Rodrigo Vivi <rodrigo.vivi@intel.com>"
# gpg: aka "Rodrigo Vivi <rodrigo.vivi@gmail.com>"
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg: There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 6D20 7068 EEDD 6509 1C2C E2A3 FA62 5F64 0EEB 13CA
# Conflicts:
# drivers/gpu/drm/i915/intel_dp.c
# drivers/gpu/drm/i915/intel_drv.h
From: Rodrigo Vivi <rodrigo.vivi@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190114183820.GA2855@intel.com
It is often useful to check whether the DRM format info retrieved from
the DRM framebuffer matches a specific YUV planes disposition.
This introduces helpers to quickly check that a provided format info
matches a YUV format with a specific disposition, in commonly-used
terminology.
The intent of providing helpers taking the format info instead of the
fourcc alone is to avoid the overhead of iterating through all formats
when the whole format info structure is available. As a result, these
helpers are very simple so they are made inline.
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190118145133.21281-2-paul.kocialkowski@bootlin.com
drm-misc-next for 5.1:
UAPI Changes:
- New fourcc identifier for ARM Framebuffer Compression v1.3
Cross-subsystem Changes:
Core Changes:
- Reorganisation of drm_device and drm_framebuffer headers
- Cleanup of the drmP inclusion
- Fix leaks in the fb-helpers
- Allow for depth different from bpp in fb-helper fbdev emulation
- Remove drm_mode_object from drm_display_mode
Driver Changes:
- Add reflection properties to rockchip
- a bunch of fixes for virtio
- a bunch of fixes for dp_mst and drivers using it, and introduction of a
new refcounting scheme
- Convertion of bochs to atomic and generic fbdev emulation
- Allow meson to remove the firmware framebuffers
[airlied: patch rcar-du to add drm_modes.h]
Signed-off-by: Dave Airlie <airlied@redhat.com>
From: Maxime Ripard <maxime.ripard@bootlin.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190116200428.u2n4jbk4mzza7n6e@flea
This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
handler. All flushing will now happen in the pipe functions.
Also enable the damage plane property for all except repaper which can
only do full updates.
ili9225:
This change made ili9225_init() equal to mipi_dbi_init() so use it.
v3: Include vblank header (Sam)
ili9225 and st7586 can't use mipi_dbi_enable_flush() (David)
v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
(kbuild test robot)
Cc: David Lechner <david@lechnology.com>
Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Tested-by: David Lechner <david@lechnology.com>
Reviewed-by: David Lechner <david@lechnology.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190115043643.2364-5-noralf@tronnes.org
The kerneldoc comment for struct dma_fence_array lacks a description
of the "work" member, leading to this docs-build warning:
./include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
Add a description and make the warning go away.
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190116153436.3b244cda@lwn.net
0-DAY reported the following bug:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 21376e2c3c
commit: e9eafcb589 [1/2] drm: move drm_can_sleep() to drm_util.h
config: alpha-allmodconfig (attached as .config)
...
In file included from include/linux/irqflags.h:16:0,
from include/drm/drm_util.h:35,
from drivers/gpu/drm/qxl/qxl_cmd.c:28:
>> arch/alpha/include/asm/irqflags.h:58:15: error: unknown type name 'bool'
static inline bool arch_irqs_disabled_flags(unsigned long flags)
^~~~
And later following bug:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 21376e2c3c
commit: e9eafcb589 [1/2] drm: move drm_can_sleep() to drm_util.h
config: ia64-allyesconfig (attached as .config)
...
In file included from arch/ia64/include/asm/irqflags.h:14,
from include/linux/irqflags.h:16,
from include/drm/drm_util.h:35,
from drivers/gpu/drm/qxl/qxl_cmd.c:28:
arch/ia64/include/asm/pal.h: In function 'ia64_pal_tr_read':
arch/ia64/include/asm/pal.h:1703:64: error: implicit declaration of function 'ia64_tpa'; did you mean 'ia64_pal'? [-Werror=implicit-function-declaration]
PAL_CALL_PHYS_STK(iprv, PAL_VM_TR_READ, reg_num, tr_type,(u64)ia64_tpa(tr_buffer));
^~~~~~~~
...
So we have a situation where we do not pull in <linux/types.h>
when building for alpha and for ia64 we need even more definitions
are required.
Two invasive fixes where considered:
- Change all declarations of arch_irqs_disabled_flags() to use bool
- Add include of <linux/types.h> to all files that uses bool for
arch_irqs_disabled_flags
To invasive with a too high pain/benefit ratio, so dropped.
They would not cover ia64 either.
Some less invasive fixes was also considered:
- Add include of <linux/types.h> to drm_util.h
- Add include of <linux/interrupt.h> to drm_util.h
The first was dropped as this did not cover the ia64 case.
The latter was considered the best option as there could
be other similar cases and we would like the header files below
include/drm/ to be selfcontained.
So we end up pulling in a lot of stuff not needed, but this is
the price we pay in drm/ because the kernel headers are not all
selfcontained.
While at it, ordred the includefiles in drm_util in alphabetical order.
Build tested with alpha,ia64,arm,x86 with allmodconfig and allyesconfig.
v2:
- fix ia64 build, changed to include interrupt.h
- sort include files alphabetically
Fixes: 733748ac37b45 ("drm: move drm_can_sleep() to drm_util.h")
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190115214845.8117-1-sam@ravnborg.org
On i965gm we need to adjust max_vblank_count dynamically
depending on whether the TV encoder is used or not. To
that end add a per-crtc max_vblank_count that takes
precedence over its device wide counterpart. The driver
can now call drm_crtc_set_max_vblank_count() to configure
the per-crtc value before calling drm_vblank_on().
Also looks like there was some discussion about exynos needing
similar treatment.
v2: Drop the extra max_vblank_count!=0 check for the
WARN(last!=current), will take care of it in i915 code (Daniel)
WARN_ON(!inmodeset) (Daniel)
WARN_ON(dev->max_vblank_count)
Pimp up the docs (Daniel)
Cc: stable@vger.kernel.org
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20181127182004.28885-1-ville.syrjala@linux.intel.com
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
The mode and ajusted_mode passed to the bridge .mode_set() operation
should never be modified by the bridge (and are not in any of the
existing bridge drivers). Make them const to make this clear.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
drm-next has been forwarded to 5.0-rc1, and we need it to apply the damage
helper for dirtyfb series from Noralf Trønnes.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
There has been a TODO waiting for quite a long time in
drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release.
*/
That's not the only reason we should fix this: forcing the driver to
track the VCPI allocations throughout a state's atomic check is
error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and
drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
idempotency. Currently the only driver actually using these helpers,
i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would
over-allocate VCPI slots, something I learned trying to implement
fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which
must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these
/must/ be called by /all/ atomic drivers supporting MST.
Changes since v9:
* Add some missing changes that were requested by danvet that I forgot
about after I redid all of the kref stuff:
* Remove unnecessary state changes in intel_dp_mst_atomic_check
* Cleanup atomic check logic for VCPI allocations - all we need to check in
compute_config is whether or not this state disables a CRTC, then free
VCPI based off that
Changes since v8:
* Fix compile errors, whoops!
Changes since v7:
- Don't check for mixed stale/valid VCPI allocations, just rely on
connector registration to stop such erroneous modesets
Changes since v6:
- Keep a kref to all of the ports we have allocations on. This required
a good bit of changing to when we call drm_dp_find_vcpi_slots(),
mainly that we need to ensure that we only redo VCPI allocations on
actual mode or CRTC changes, not crtc_state->active changes.
Additionally, we no longer take the registration of the DRM connector
for each port into account because so long as we have a kref to the
port in the new or previous atomic state, the connector will stay
registered.
- Use the small changes to drm_dp_put_port() to add even more error
checking to make misusage of the helpers more obvious. I added this
after having to chase down various use-after-free conditions that
started popping up from the new helpers so no one else has to
troubleshoot that.
- Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC()
- Update documentation again, note that find/release() should both not be
called on the same port in a single atomic check phase (but multiple
calls to one or the other is OK)
Changes since v4:
- Don't skip the atomic checks for VCPI allocations if no new VCPI
allocations happen in a state. This makes the next change I'm about
to list here a lot easier to implement.
- Don't ignore VCPI allocations on destroyed ports, instead ensure that
when ports are destroyed and still have VCPI allocations in the
topology state, the only state changes allowed are releasing said
ports' VCPI. This prevents a state with a mix of VCPI allocations
from destroyed ports, and allocations from valid ports.
Changes since v3:
- Don't release VCPI allocations in the topology state immediately in
drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip
over them in drm_dp_mst_duplicate_state(). This makes it so
drm_dp_atomic_release_vcpi_slots() is still idempotent while also
throwing warnings if the driver messes up it's book keeping and tries
to release VCPI slots on a port that doesn't have any pre-existing
VCPI allocation - danvet
- Change mst_state/state in some debugging messages to "mst state"
Changes since v2:
- Use kmemdup() for duplicating MST state - danvet
- Move port validation out of duplicate state callback - danvet
- Handle looping through MST topology states in
drm_dp_mst_atomic_check() so the driver doesn't have to do it
- Fix documentation in drm_dp_atomic_find_vcpi_slots()
- Move the atomic check for each individual topology state into it's
own function, reduces indenting
- Don't consider "stale" MST ports when calculating the bandwidth
requirements. This is needed because originally we relied on the
state duplication functions to prune any stale ports from the new
state, which would prevent us from incorrectly considering their
bandwidth requirements alongside legitimate new payloads.
- Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
- Annotate atomic VCPI and atomic check functions with __must_check
- danvet
Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook,
just give drivers a function to call themselves
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-19-lyude@redhat.com
The current way of handling refcounting in the DP MST helpers is really
confusing and probably just plain wrong because it's been hacked up many
times over the years without anyone actually going over the code and
seeing if things could be simplified.
To the best of my understanding, the current scheme works like this:
drm_dp_mst_port and drm_dp_mst_branch both have a single refcount. When
this refcount hits 0 for either of the two, they're removed from the
topology state, but not immediately freed. Both ports and branch devices
will reinitialize their kref once it's hit 0 before actually destroying
themselves. The intended purpose behind this is so that we can avoid
problems like not being able to free a remote payload that might still
be active, due to us having removed all of the port/branch device
structures in memory, as per:
commit 91a25e4631 ("drm/dp/mst: deallocate payload on port destruction")
Which may have worked, but then it caused use-after-free errors. Being
new to MST at the time, I tried fixing it;
commit 263efde31f ("drm/dp/mst: Get validated port ref in drm_dp_update_payload_part1()")
But, that was broken: both drm_dp_mst_port and drm_dp_mst_branch structs
are validated in almost every DP MST helper function. Simply put, this
means we go through the topology and try to see if the given
drm_dp_mst_branch or drm_dp_mst_port is still attached to something
before trying to use it in order to avoid dereferencing freed memory
(something that has happened a LOT in the past with this library).
Because of this it doesn't actually matter whether or not we keep keep
the ports and branches around in memory as that's not enough, because
any function that validates the branches and ports passed to it will
still reject them anyway since they're no longer in the topology
structure. So, use-after-free errors were fixed but payload deallocation
was completely broken.
Two years later, AMD informed me about this issue and I attempted to
come up with a temporary fix, pending a long-overdue cleanup of this
library:
commit c54c7374ff ("drm/dp_mst: Skip validating ports during destruction, just ref")
But then that introduced use-after-free errors, so I quickly reverted
it:
commit 9765635b30 ("Revert "drm/dp_mst: Skip validating ports during destruction, just ref"")
And in the process, learned that there is just no simple fix for this:
the design is just broken. Unfortunately, the usage of these helpers are
quite broken as well. Some drivers like i915 have been smart enough to
avoid accessing any kind of information from MST port structures, but
others like nouveau have assumed, understandably so, that
drm_dp_mst_port structures are normal and can just be accessed at any
time without worrying about use-after-free errors.
After a lot of discussion, me and Daniel Vetter came up with a better
idea to replace all of this.
To summarize, since this is documented far more indepth in the
documentation this patch introduces, we make it so that drm_dp_mst_port
and drm_dp_mst_branch structures have two different classes of
refcounts: topology_kref, and malloc_kref. topology_kref corresponds to
the lifetime of the given drm_dp_mst_port or drm_dp_mst_branch in it's
given topology. Once it hits zero, any associated connectors are removed
and the branch or port can no longer be validated. malloc_kref
corresponds to the lifetime of the memory allocation for the actual
structure, and will always be non-zero so long as the topology_kref is
non-zero. This gives us a way to allow callers to hold onto port and
branch device structures past their topology lifetime, and dramatically
simplifies the lifetimes of both structures. This also finally fixes the
port deallocation problem, properly.
Additionally: since this now means that we can keep ports and branch
devices allocated in memory for however long we need, we no longer need
a significant amount of the port validation that we currently do.
Additionally, there is one last scenario that this fixes, which couldn't
have been fixed properly beforehand:
- CPU1 unrefs port from topology (refcount 1->0)
- CPU2 refs port in topology(refcount 0->1)
Since we now can guarantee memory safety for ports and branches
as-needed, we also can make our main reference counting functions fix
this problem by using kref_get_unless_zero() internally so that topology
refcounts can only ever reach 0 once.
Changes since v4:
* Change the kernel-figure summary for dp-mst/topology-figure-1.dot a
bit - danvet
* Remove figure numbers - danvet
Changes since v3:
* Remove rebase detritus - danvet
* Split out purely style changes into separate patches - hwentlan
Changes since v2:
* Fix commit message - checkpatch
* s/)-1/) - 1/g - checkpatch
Changes since v1:
* Remove forward declarations - danvet
* Move "Branch device and port refcounting" section from documentation
into kernel-doc comments - danvet
* Export internal topology lifetime functions into their own section in
the kernel-docs - danvet
* s/@/&/g for struct references in kernel-docs - danvet
* Drop the "when they are no longer being used" bits from the kernel
docs - danvet
* Modify diagrams to show how the DRM driver interacts with the topology
and payloads - danvet
* Make suggested documentation changes for
drm_dp_mst_topology_get_mstb() and drm_dp_mst_topology_get_port() -
danvet
* Better explain the relationship between malloc refs and topology krefs
in the documentation for drm_dp_mst_topology_get_port() and
drm_dp_mst_topology_get_mstb() - danvet
* Fix "See also" in drm_dp_mst_topology_get_mstb() - danvet
* Rename drm_dp_mst_topology_get_(port|mstb)() ->
drm_dp_mst_topology_try_get_(port|mstb)() and
drm_dp_mst_topology_ref_(port|mstb)() ->
drm_dp_mst_topology_get_(port|mstb)() - danvet
* s/should/must in docs - danvet
* WARN_ON(refcount == 0) in topology_get_(mstb|port) - danvet
* Move kdocs for mstb/port structs inline - danvet
* Split drm_dp_get_last_connected_port_and_mstb() changes into their own
commit - danvet
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Juston Li <juston.li@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190111005343.17443-7-lyude@redhat.com
