Hi,
This series address multiple issues with the transposer support, and thus the writeback support.
Let me know what you think, Maxime
Maxime Ripard (6): drm/vc4: hvs: Reset muxes at probe time drm/vc4: txp: Don't set TXP_VSTART_AT_EOF drm/vc4: txp: Force alpha to be 0xff if it's disabled drm/vc4: kms: Store channel in local variable drm/vc4: kms: Warn if we have an incompatible muxing setup drm/vc4: kms: Improve logging
drivers/gpu/drm/vc4/vc4_hvs.c | 26 +++++++++++++++++++----- drivers/gpu/drm/vc4/vc4_kms.c | 38 +++++++++++++++++++++++++++-------- drivers/gpu/drm/vc4/vc4_txp.c | 4 +++- 3 files changed, 54 insertions(+), 14 deletions(-)
By default, the HVS driver will force the HVS output 3 to be muxed to the HVS channel 2. However, the Transposer can only be assigned to the HVS channel 2, so whenever we try to use the writeback connector, we'll mux its associated output (Output 2) to the channel 2.
This leads to both the output 2 and 3 feeding from the same channel, which is explicitly discouraged in the documentation.
In order to avoid this, let's reset all the output muxes to their reset value.
Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hvs.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 604933e20e6a..911968a1c97b 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -582,6 +582,7 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) struct vc4_hvs *hvs = NULL; int ret; u32 dispctrl; + u32 reg;
hvs = devm_kzalloc(&pdev->dev, sizeof(*hvs), GFP_KERNEL); if (!hvs) @@ -653,6 +654,26 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
vc4->hvs = hvs;
+ reg = HVS_READ(SCALER_DISPECTRL); + reg &= ~SCALER_DISPECTRL_DSP2_MUX_MASK; + HVS_WRITE(SCALER_DISPECTRL, + reg | VC4_SET_FIELD(0, SCALER_DISPECTRL_DSP2_MUX)); + + reg = HVS_READ(SCALER_DISPCTRL); + reg &= ~SCALER_DISPCTRL_DSP3_MUX_MASK; + HVS_WRITE(SCALER_DISPCTRL, + reg | VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX)); + + reg = HVS_READ(SCALER_DISPEOLN); + reg &= ~SCALER_DISPEOLN_DSP4_MUX_MASK; + HVS_WRITE(SCALER_DISPEOLN, + reg | VC4_SET_FIELD(3, SCALER_DISPEOLN_DSP4_MUX)); + + reg = HVS_READ(SCALER_DISPDITHER); + reg &= ~SCALER_DISPDITHER_DSP5_MUX_MASK; + HVS_WRITE(SCALER_DISPDITHER, + reg | VC4_SET_FIELD(3, SCALER_DISPDITHER_DSP5_MUX)); + dispctrl = HVS_READ(SCALER_DISPCTRL);
dispctrl |= SCALER_DISPCTRL_ENABLE; @@ -660,10 +681,6 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) SCALER_DISPCTRL_DISPEIRQ(1) | SCALER_DISPCTRL_DISPEIRQ(2);
- /* Set DSP3 (PV1) to use HVS channel 2, which would otherwise - * be unused. - */ - dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK; dispctrl &= ~(SCALER_DISPCTRL_DMAEIRQ | SCALER_DISPCTRL_SLVWREIRQ | SCALER_DISPCTRL_SLVRDEIRQ | @@ -677,7 +694,6 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) SCALER_DISPCTRL_DSPEISLUR(1) | SCALER_DISPCTRL_DSPEISLUR(2) | SCALER_DISPCTRL_SCLEIRQ); - dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
HVS_WRITE(SCALER_DISPCTRL, dispctrl);
The TXP_VSTART_AT_EOF will generate a second VSTART signal to the HVS. However, the HVS waits for VSTART to enable the FIFO and will thus start filling the FIFO before the start of the frame.
This leads to corruption at the beginning of the first frame, and content from the previous frame at the beginning of the next frames.
Since one VSTART is enough, let's get rid of it.
Fixes: 008095e065a8 ("drm/vc4: Add support for the transposer block") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_txp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 9809ca3e2945..ace2d03649ba 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -298,7 +298,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, if (WARN_ON(i == ARRAY_SIZE(drm_fmts))) return;
- ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI | + ctrl = TXP_GO | TXP_EI | VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE) | VC4_SET_FIELD(txp_fmts[i], TXP_FORMAT);
If we use a format that has padding instead of the alpha component (such as XRGB8888), it appears that the Transposer will fill the padding to 0, disregarding what was stored in the input buffer padding.
This leads to issues with IGT, since it will set the padding to 0xff, but will then compare the CRC of the two frames which will thus fail. Another nice side effect is that it is now possible to just use the buffer as ARGB.
Fixes: 008095e065a8 ("drm/vc4: Add support for the transposer block") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_txp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index ace2d03649ba..5b4dd644214f 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -304,6 +304,8 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
if (fb->format->has_alpha) ctrl |= TXP_ALPHA_ENABLE; + else + ctrl |= TXP_ALPHA_INVERT;
gem = drm_fb_cma_get_gem_obj(fb, 0); TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]);
Hi Maxime
Am 28.03.22 um 17:36 schrieb Maxime Ripard:
If we use a format that has padding instead of the alpha component (such as XRGB8888), it appears that the Transposer will fill the padding to 0, disregarding what was stored in the input buffer padding.
This leads to issues with IGT, since it will set the padding to 0xff, but will then compare the CRC of the two frames which will thus fail. Another nice side effect is that it is now possible to just use the buffer as ARGB.
Fixes: 008095e065a8 ("drm/vc4: Add support for the transposer block") Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_txp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index ace2d03649ba..5b4dd644214f 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -304,6 +304,8 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
if (fb->format->has_alpha) ctrl |= TXP_ALPHA_ENABLE;
- else
ctrl |= TXP_ALPHA_INVERT;
'Invert' is somewhat misleading here, but your commit message nicely explains what this code does. Maybe add a short comment with the explanation.
Best regards Thomas
gem = drm_fb_cma_get_gem_obj(fb, 0); TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]);
We use the channel from our vc4_crtc_state structure in multiple places, let's store it in a local variable to make it cleaner.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 24de29bc1cda..94c58ec37a27 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -279,13 +279,14 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, for_each_new_crtc_in_state(state, crtc, crtc_state, i) { struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + unsigned int channel = vc4_state->assigned_channel;
if (!vc4_state->update_muxing) continue;
switch (vc4_crtc->data->hvs_output) { case 2: - mux = (vc4_state->assigned_channel == 2) ? 0 : 1; + mux = (channel == 2) ? 0 : 1; reg = HVS_READ(SCALER_DISPECTRL); HVS_WRITE(SCALER_DISPECTRL, (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) | @@ -293,10 +294,10 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, break;
case 3: - if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED) + if (channel == VC4_HVS_CHANNEL_DISABLED) mux = 3; else - mux = vc4_state->assigned_channel; + mux = channel;
reg = HVS_READ(SCALER_DISPCTRL); HVS_WRITE(SCALER_DISPCTRL, @@ -305,10 +306,10 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, break;
case 4: - if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED) + if (channel == VC4_HVS_CHANNEL_DISABLED) mux = 3; else - mux = vc4_state->assigned_channel; + mux = channel;
reg = HVS_READ(SCALER_DISPEOLN); HVS_WRITE(SCALER_DISPEOLN, @@ -318,10 +319,10 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, break;
case 5: - if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED) + if (channel == VC4_HVS_CHANNEL_DISABLED) mux = 3; else - mux = vc4_state->assigned_channel; + mux = channel;
reg = HVS_READ(SCALER_DISPDITHER); HVS_WRITE(SCALER_DISPDITHER,
The documentation explicitly states we must prevent the output 2 and 3 from feeding from the same HVS channel.
Let's add a warning to make some noise if we ever find ourselves in such a case.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 94c58ec37a27..d94f78eac936 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -286,6 +286,9 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
switch (vc4_crtc->data->hvs_output) { case 2: + WARN_ON(VC4_GET_FIELD(HVS_READ(SCALER_DISPCTRL), + SCALER_DISPCTRL_DSP3_MUX) == channel); + mux = (channel == 2) ? 0 : 1; reg = HVS_READ(SCALER_DISPECTRL); HVS_WRITE(SCALER_DISPECTRL,
Am 28.03.22 um 17:36 schrieb Maxime Ripard:
The documentation explicitly states we must prevent the output 2 and 3 from feeding from the same HVS channel.
Let's add a warning to make some noise if we ever find ourselves in such a case.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_kms.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 94c58ec37a27..d94f78eac936 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -286,6 +286,9 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
switch (vc4_crtc->data->hvs_output) { case 2:
WARN_ON(VC4_GET_FIELD(HVS_READ(SCALER_DISPCTRL),SCALER_DISPCTRL_DSP3_MUX) == channel);
Should be drm_WARN_ON().
Is that something that could be detected during atomic-check steps?
Best regards Thomas
mux = (channel == 2) ? 0 : 1; reg = HVS_READ(SCALER_DISPECTRL); HVS_WRITE(SCALER_DISPECTRL,
Hi Thomas,
On Wed, Apr 06, 2022 at 11:07:53AM +0200, Thomas Zimmermann wrote:
Am 28.03.22 um 17:36 schrieb Maxime Ripard:
The documentation explicitly states we must prevent the output 2 and 3 from feeding from the same HVS channel.
Let's add a warning to make some noise if we ever find ourselves in such a case.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_kms.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 94c58ec37a27..d94f78eac936 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -286,6 +286,9 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, switch (vc4_crtc->data->hvs_output) { case 2:
WARN_ON(VC4_GET_FIELD(HVS_READ(SCALER_DISPCTRL),SCALER_DISPCTRL_DSP3_MUX) == channel);Should be drm_WARN_ON().
Indeed, thanks
Is that something that could be detected during atomic-check steps?
Atomic_check will allocate the output and take into account these constraints. However, what was happening here was that the hardware already had a default value that caused the conflict.
Patch 1 fixed it so we should never be in that condition, but better be safe than sorry.
Maxime
When debugging, finding out what muxing decisions were made and what the actual core clock rate is is always useful, so let's add some more messages.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index d94f78eac936..29ecc34a4069 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -421,6 +421,9 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) new_hvs_state->core_clock_rate);
clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate); + + drm_dbg(dev, "Core clock actual rate: %lu Hz\n", + clk_get_rate(hvs->core_clk)); } }
@@ -787,9 +790,18 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, unsigned int matching_channels; unsigned int channel;
+ drm_dbg(dev, "%s: Trying to find a channel.\n", crtc->name); + /* Nothing to do here, let's skip it */ - if (old_crtc_state->enable == new_crtc_state->enable) + if (old_crtc_state->enable == new_crtc_state->enable) { + if (new_crtc_state->enable) + drm_dbg(dev, "%s: Already enabled, reusing channel %d.\n", + crtc->name, new_vc4_crtc_state->assigned_channel); + else + drm_dbg(dev, "%s: Disabled, ignoring.\n", crtc->name); + continue; + }
/* Muxing will need to be modified, mark it as such */ new_vc4_crtc_state->update_muxing = true; @@ -797,6 +809,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, /* If we're disabling our CRTC, we put back our channel */ if (!new_crtc_state->enable) { channel = old_vc4_crtc_state->assigned_channel; + + drm_dbg(dev, "%s: Disabling, Freeing channel %d\n", + crtc->name, channel); + hvs_new_state->fifo_state[channel].in_use = false; new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; continue; @@ -831,6 +847,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, return -EINVAL;
channel = ffs(matching_channels) - 1; + + drm_dbg(dev, "Assigned HVS channel %d to CRTC %s\n", channel, crtc->name); new_vc4_crtc_state->assigned_channel = channel; unassigned_channels &= ~BIT(channel); hvs_new_state->fifo_state[channel].in_use = true;
Hi
Am 28.03.22 um 17:36 schrieb Maxime Ripard:
Hi,
This series address multiple issues with the transposer support, and thus the writeback support.
With my comments considered, feel free to add
Acked-by: Thomas Zimmermann tzimmermann@suse.de
I cannot really check the correctness of the individual HW operations though.
Best regards Thomas
Let me know what you think, Maxime
Maxime Ripard (6): drm/vc4: hvs: Reset muxes at probe time drm/vc4: txp: Don't set TXP_VSTART_AT_EOF drm/vc4: txp: Force alpha to be 0xff if it's disabled drm/vc4: kms: Store channel in local variable drm/vc4: kms: Warn if we have an incompatible muxing setup drm/vc4: kms: Improve logging
drivers/gpu/drm/vc4/vc4_hvs.c | 26 +++++++++++++++++++----- drivers/gpu/drm/vc4/vc4_kms.c | 38 +++++++++++++++++++++++++++-------- drivers/gpu/drm/vc4/vc4_txp.c | 4 +++- 3 files changed, 54 insertions(+), 14 deletions(-)
On Wed, Apr 06, 2022 at 11:10:12AM +0200, Thomas Zimmermann wrote:
Hi
Am 28.03.22 um 17:36 schrieb Maxime Ripard:
Hi,
This series address multiple issues with the transposer support, and thus the writeback support.
With my comments considered, feel free to add
Acked-by: Thomas Zimmermann tzimmermann@suse.de
I cannot really check the correctness of the individual HW operations though.
I amended the patches with your suggestions and pushed them, thanks! Maxime
dri-devel@lists.freedesktop.org
-
Maxime Ripard -
Thomas Zimmermann