| Message ID | 20200923084032.218619-2-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/vc4: crtc: Rework a bit the CRTC state code | expand |
Hi Maxime Sorry for the delay. On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > 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> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> I retested too and had triple output (HDMI-0, HDMI-1, and DPI) working happily! Switching around resolutions on the HDMI outputs was working fine. DPI was an Adafruit Kippah and 800x480 LCD, so no options on resolution. We may finally have this muxing nailed. Dave > --- > > Changes from v1: > - Split away the crtc state reset refactoring > - Fixed the checkpatch warnings > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 1 + > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ > 3 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index 7ef20adedee5..482219fb4db2 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) > return; > } > > + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > } > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index 8c8d96b6289f..90b911fd2a7f 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -532,6 +532,8 @@ struct vc4_crtc_state { > } 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..149825ff5df8 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,8 @@ 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 +639,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 +684,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; > -- > 2.26.2 >
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 7ef20adedee5..482219fb4db2 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) return; } + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 8c8d96b6289f..90b911fd2a7f 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -532,6 +532,8 @@ struct vc4_crtc_state { } 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..149825ff5df8 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,8 @@ 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 +639,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 +684,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;
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> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- Changes from v1: - Split away the crtc state reset refactoring - Fixed the checkpatch warnings --- drivers/gpu/drm/vc4/vc4_crtc.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-)