Hi Maxime
Thanks for the patch - it makes mode switching reliable for me! :-)
On Fri, 18 Sep 2020 at 15:59, Maxime Ripard <maxime@xxxxxxxxxx> 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@xxxxxxxxxx>
Tested-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
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_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;
> --
> 2.26.2
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
