Hi Maxime
Thanks for the patch - it makes mode switching reliable for me! :-)
On Fri, 18 Sep 2020 at 15:59, Maxime Ripard 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
Tested-by: Dave Stevenson
Review comments below though.
> ---
> 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;
This error path has me worried, but I may have missed it.
If crtc->state was set then it's been freed via
vc4_crtc_destroy_state. However I don't see anything that has reset
crtc->state to NULL.
If the new alloc fails then I believe we exit with crtc->state still
set to the old freed pointer.
The old code directly set crtc->state, so it got reset to NULL by the failure.
> +
> + 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)
Checkpatch whinge on that
CHECK: No space is necessary after a cast
#55: FILE: drivers/gpu/drm/vc4/vc4_drv.h:539:
+#define VC4_HVS_CHANNEL_DISABLED ((unsigned int) -1)
CHECK: Please use a blank line after function/struct/union/enum declarations
#55: FILE: drivers/gpu/drm/vc4/vc4_drv.h:539:
};
+#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;
Blank line between variables and code, or not in this subsystem?
Checkpatch hasn't complained to me here.
> 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_channel