From 3ac3a9f4427ec47ae3f42305754a60443f19938e Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Thu, 8 Oct 2020 13:25:16 +0200 Subject: [PATCH 1/6] drm/vc4: kms: Split the HVS muxing check in a separate function The code that assigns HVS channels during atomic_check is starting to grow a bit big, let's move it into a separate function. Signed-off-by: Maxime Ripard Reviewed-by: Hoegeun Kwon Tested-by: Hoegeun Kwon --- drivers/gpu/drm/vc4/vc4_kms.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 09828110fc8e17..7bc7e16a8ceb6f 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -618,14 +618,14 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { #define NUM_OUTPUTS 6 #define NUM_CHANNELS 3 -static int -vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) +static int vc4_pv_muxing_atomic_check(struct drm_device *dev, + struct drm_atomic_state *state) { unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct vc4_dev *vc4 = to_vc4_dev(state->dev); struct drm_crtc *crtc; - int i, ret; + unsigned int i; /* * Since the HVS FIFOs are shared across all the pixelvalves and @@ -701,6 +701,18 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) } } + return 0; +} + +static int +vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) +{ + int ret; + + ret = vc4_pv_muxing_atomic_check(dev, state); + if (ret) + return ret; + ret = vc4_ctm_atomic_check(dev, state); if (ret < 0) return ret; From 0183629776ae8e21b0ff2953ef070f3d63eb6d0a Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Thu, 8 Oct 2020 13:25:17 +0200 Subject: [PATCH 2/6] drm/vc4: kms: Document the muxing corner cases We've had a number of muxing corner-cases with specific ways to reproduce them, so let's document them to make sure they aren't lost and introduce regressions later on. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 7bc7e16a8ceb6f..15bfbd7bef80e4 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -618,6 +618,28 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { #define NUM_OUTPUTS 6 #define NUM_CHANNELS 3 +/* + * The BCM2711 HVS has up to 7 output connected to the pixelvalves and + * the TXP (and therefore all the CRTCs found on that platform). + * + * The naive (and our initial) implementation would just iterate over + * all the active CRTCs, try to find a suitable FIFO, and then remove it + * from the available FIFOs pool. However, there's a few corner cases + * that need to be considered: + * + * - When running in a dual-display setup (so with two CRTCs involved), + * we can update the state of a single CRTC (for example by changing + * its mode using xrandr under X11) without affecting the other. In + * this case, the other CRTC wouldn't be in the state at all, so we + * need to consider all the running CRTCs in the DRM device to assign + * a FIFO, not just the one in the state. + * + * - Since we need the pixelvalve to be disabled and enabled back when + * the FIFO is changed, we should keep the FIFO assigned for as long + * as the CRTC is enabled, only considering it free again once that + * CRTC has been disabled. This can be tested by booting X11 on a + * single display, and changing the resolution down and then back up. + */ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { From 3c76878663897a2a508cf3e0c95e7ae45ac9aefc Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Thu, 8 Oct 2020 13:25:18 +0200 Subject: [PATCH 3/6] drm/vc4: kms: Don't disable the muxing of an active CRTC The current HVS muxing code will consider the CRTCs in a given state to setup their muxing in the HVS, and disable the other CRTCs muxes. However, it's valid to only update a single CRTC with a state, and in this situation we would mux out a CRTC that was enabled but left untouched by the new state. Fix this by considering all the CRTCs in the state and the ones with a state and active. Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_kms.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 15bfbd7bef80e4..6edb4575cf59e4 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -188,6 +188,23 @@ static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4, } } +static struct drm_crtc_state * +drm_atomic_get_new_or_current_crtc_state(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (crtc_state) + return crtc_state; + + return crtc->state; +} + +#define for_each_new_or_current_crtc_state(__state, crtc, crtc_state) \ + list_for_each_entry(crtc, &__state->dev->mode_config.crtc_list, head) \ + for_each_if(crtc_state = drm_atomic_get_new_or_current_crtc_state(__state, crtc)) + static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, struct drm_atomic_state *state) { @@ -197,16 +214,16 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, unsigned char dsp3_mux = 3; unsigned char dsp4_mux = 3; unsigned char dsp5_mux = 3; - unsigned int i; u32 reg; - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state); + for_each_new_or_current_crtc_state(state, crtc, crtc_state) { struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + struct vc4_crtc_state *vc4_state; if (!crtc_state->active) continue; + vc4_state = to_vc4_crtc_state(crtc_state); switch (vc4_crtc->data->hvs_output) { case 2: dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1; From 864f1732efd4d54a14e0aa1a0c4eb5efa3c84e5d Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Thu, 8 Oct 2020 13:25:19 +0200 Subject: [PATCH 4/6] drm/vc4: kms: Fix VBLANK reporting on a disabled CRTC If a CRTC is enabled but not active, and that we're then doing a page flip on another CRTC, drm_atomic_get_crtc_state will bring the first CRTC state into the global state, and will make us wait for its vblank as well, even though that might never occur. Fix this by considering all the enabled CRTCs by either using their new state in the global state, or using their current state if they aren't part of the new state being checked, to remove their assigned channel from the pool before started to assign channels to CRTCs enabled by the state. Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_kms.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 6edb4575cf59e4..64ced42fbaff2c 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -651,6 +651,14 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = { * need to consider all the running CRTCs in the DRM device to assign * a FIFO, not just the one in the state. * + * - To fix the above, we can't use drm_atomic_get_crtc_state on all + * enabled CRTCs to pull their CRTC state into the global state, since + * a page flip would start considering their vblank to complete. Since + * we don't have a guarantee that they are actually active, that + * vblank might never happen, and shouldn't even be considered if we + * want to do a page flip on a single CRTC. That can be tested by + * doing a modetest -v first on HDMI1 and then on HDMI0. + * * - Since we need the pixelvalve to be disabled and enabled back when * the FIFO is changed, we should keep the FIFO assigned for as long * as the CRTC is enabled, only considering it free again once that @@ -663,6 +671,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct vc4_dev *vc4 = to_vc4_dev(state->dev); + struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; unsigned int i; @@ -674,15 +683,14 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, * the same CRTCs, instead of evaluating only the CRTC being * modified. */ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - struct drm_crtc_state *crtc_state; + for_each_new_or_current_crtc_state(state, crtc, crtc_state) { + struct vc4_crtc_state *vc4_crtc_state; - if (!crtc->state->enable) + if (!crtc_state->enable) continue; - crtc_state = drm_atomic_get_crtc_state(state, crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); + vc4_crtc_state = to_vc4_crtc_state(crtc_state); + unassigned_channels &= ~BIT(vc4_crtc_state->assigned_channel); } for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { @@ -700,10 +708,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, if (!new_crtc_state->enable) continue; - if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) { - unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel); + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) continue; - } /* * The problem we have to solve here is that we have From b019e1194b7fea1f86d5ef28bb6cb94f17eb7be6 Mon Sep 17 00:00:00 2001 From: Phil Elwell Date: Wed, 31 Jul 2019 17:36:34 +0100 Subject: [PATCH 5/6] drm/vc4: A present but empty dmas disables audio Overlays are unable to remove properties in the base DTB, but they can overwrite them. Allow a present but empty 'dmas' property to also disable the HDMI audio interface. See: https://github.com/raspberrypi/linux/issues/2489 Signed-off-by: Phil Elwell --- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 685dc64068406e..efecd5fc051803 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1780,10 +1780,12 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) const __be32 *addr; int index; int ret; + int len; - if (!of_find_property(dev->of_node, "dmas", NULL)) { + if (!of_find_property(dev->of_node, "dmas", &len) || + len == 0) { dev_warn(dev, - "'dmas' DT property is missing, no HDMI audio\n"); + "'dmas' DT property is missing or empty, no HDMI audio\n"); return 0; } From 1f9e623f4734e3689767cb1dc13e9e00843f4957 Mon Sep 17 00:00:00 2001 From: Dave Stevenson Date: Tue, 6 Oct 2020 18:44:42 +0100 Subject: [PATCH 6/6] drm/vc4: Add debugfs node that dumps the current display lists This allows easy analysis of display lists when debugging. Signed-off-by: Dave Stevenson --- drivers/gpu/drm/vc4/vc4_hvs.c | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 22403ab2a4305b..b7e2e68355c4b1 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -95,6 +95,45 @@ static int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data) return 0; } +static int vc4_hvs_debugfs_dlist(struct seq_file *m, void *data) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct drm_printer p = drm_seq_file_printer(m); + unsigned int next_entry_start = 0; + unsigned int i, j; + u32 dlist_word, dispstat; + + for (i = 0; i < SCALER_CHANNELS_COUNT; i++) { + dispstat = VC4_GET_FIELD(HVS_READ(SCALER_DISPSTATX(i)), + SCALER_DISPSTATX_MODE); + if (dispstat == SCALER_DISPSTATX_MODE_DISABLED || + dispstat == SCALER_DISPSTATX_MODE_EOF) { + drm_printf(&p, "HVS chan %u disabled\n", i); + continue; + } + + drm_printf(&p, "HVS chan %u:\n", i); + + for (j = HVS_READ(SCALER_DISPLISTX(i)); j < 256; j++) { + dlist_word = readl((u32 __iomem *)vc4->hvs->dlist + j); + drm_printf(&p, "dlist: %02d: 0x%08x\n", j, + dlist_word); + if (!next_entry_start || + next_entry_start == j) { + if (dlist_word & SCALER_CTL0_END) + break; + next_entry_start = j + + VC4_GET_FIELD(dlist_word, + SCALER_CTL0_SIZE); + } + } + } + + return 0; +} + /* The filter kernel is composed of dwords each containing 3 9-bit * signed integers packed next to each other. */ @@ -673,6 +712,8 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) vc4_debugfs_add_regset32(drm, "hvs_regs", &hvs->regset); vc4_debugfs_add_file(drm, "hvs_underrun", vc4_hvs_debugfs_underrun, NULL); + vc4_debugfs_add_file(drm, "hvs_dlists", vc4_hvs_debugfs_dlist, + NULL); return 0; }