The HVS FIFOs are currently assigned each time we have an atomic_check for all the enabled CRTCs.

However, if we are running multiple outputs in parallel and we happen to disable the first (by index) CRTC, we end up changing the assigned FIFO of the second CRTC without disabling and reenabling the pixelvalve which ends up in a stall and eventually a VBLANK timeout.

In order to fix this, we can create a special value for our assigned channel to mark it as disabled, and if our CRTC already had an assigned channel in its previous state, we keep on using it.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- drivers/gpu/drm/vc4/vc4_drv.h | 1 + drivers/gpu/drm/vc4/vc4_kms.c | 21 +++++++++++++++------ 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index a393f93390a2..be754120faa8 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc,

void vc4_crtc_reset(struct drm_crtc *crtc) { + struct vc4_crtc_state *vc4_crtc_state; + if (crtc->state) vc4_crtc_destroy_state(crtc, crtc->state); - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); - if (crtc->state) - __drm_atomic_helper_crtc_reset(crtc, crtc->state); + + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); + if (!vc4_crtc_state) + return; + + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; + crtc->state = &vc4_crtc_state->base; + __drm_atomic_helper_crtc_reset(crtc, crtc->state); }

static const struct drm_crtc_funcs vc4_crtc_funcs = { diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 8c8d96b6289f..2b13f2126f13 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -531,6 +531,7 @@ struct vc4_crtc_state { unsigned int bottom; } margins; }; +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int) -1)

static inline struct vc4_crtc_state * to_vc4_crtc_state(struct drm_crtc_state *crtc_state) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 01fa60844695..f452dad50c22 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -616,7 +616,7 @@ static int vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; int i, ret;

@@ -629,6 +629,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) * modified. */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + struct drm_crtc_state *crtc_state; if (!crtc->state->enable) continue;

@@ -637,15 +638,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) return PTR_ERR(crtc_state); }

- for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - struct vc4_crtc_state *vc4_crtc_state = - to_vc4_crtc_state(crtc_state); + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + struct vc4_crtc_state *new_vc4_crtc_state = + to_vc4_crtc_state(new_crtc_state); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels;

- if (!crtc_state->enable) + if (old_crtc_state->enable && !new_crtc_state->enable) + new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; + + 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); + continue; + } + /* * The problem we have to solve here is that we have * up to 7 encoders, connected to up to 6 CRTCs. @@ -674,7 +683,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (matching_channels) { unsigned int channel = ffs(matching_channels) - 1;

- vc4_crtc_state->assigned_channel = channel; + new_vc4_crtc_state->assigned_channel = channel; unassigned_channels &= ~BIT(channel); } else { return -EINVAL;

Hi Maxime

Thanks for the patch.

On Fri, 18 Sep 2020 at 15:59, Maxime Ripard maxime@cerno.tech wrote:

The HVS has three FIFOs that can be assigned to a number of PixelValves through a mux.

However, changing that FIFO requires that we disable and then enable the pixelvalve, so we want to assign FIFOs to all the enabled CRTCs, and not just the active ones.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech

Tested-by: Dave Stevenson dave.stevenson@raspberrypi.com Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com

---

drivers/gpu/drm/vc4/vc4_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index af3ee3dcdab6..01fa60844695 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -643,7 +643,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels;

•           if (!crtc_state->active)
  

•           if (!crtc_state->enable)
                    continue;
  
            /*
  

-- 2.26.2

Hi Maxime,

On 9/18/20 11:59 PM, Maxime Ripard wrote:

The HVS has three FIFOs that can be assigned to a number of PixelValves through a mux.

However, changing that FIFO requires that we disable and then enable the pixelvalve, so we want to assign FIFOs to all the enabled CRTCs, and not just the active ones.

Thanks for the patch.

There is a problem when doing page flip. After connecting 2 HDMIs without hotplug and booting.(Same thing when using hotplug.)

When executing page flip for each of HDMI 0 and 1 in modetest operation does not work normally. (crtc irq does not occur, so timeout occurs.) Sometimes both hdmi 0 or 1 work or not.

LOGs: root:~> modetest -Mvc4 -s 32:1280x720 -v setting mode 1280x720-60Hz@XR24 on connectors 32, crtc 64 failed to set gamma: Invalid argument freq: 60.24Hz freq: 60.00Hz

root:~> modetest -Mvc4 -s 38:1280x720 -v setting mode 1280x720-60Hz@XR24 on connectors 38, crtc 69 failed to set gamma: Invalid argument select timed out or error (ret 0) select timed out or error (ret 0)

Could you please check it?

Best regards, Hoegeun

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech

---

drivers/gpu/drm/vc4/vc4_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index af3ee3dcdab6..01fa60844695 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -643,7 +643,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels;

• if (!crtc_state->active)
  

• if (!crtc_state->enable)
  continue;
  
  

  /*