[PATCH] drm/vc4: hvs: Pull the state of all the CRTCs prior to PV muxing

[Hoegeun Kwon]

Hi Maxime,

On 9/17/20 9:16 PM, Maxime Ripard wrote:
> The vc4 display engine has a first controller called the HVS that will
> perform the composition of the planes. That HVS has 3 FIFOs and can
> therefore compose planes for up to three outputs. The timings part is
> generated through a component called the Pixel Valve, and the BCM2711 has 6
> of them.
>
> Thus, the HVS has some bits to control which FIFO gets output to which
> Pixel Valve. The current code supports that muxing by looking at all the
> CRTCs in a new DRM atomic state in atomic_check, and given the set of
> contraints that we have, assigns FIFOs to CRTCs or reject the mode
> entirely. The actual muxing will occur during atomic_commit.
>
> However, that doesn't work if only a fraction of the CRTCs' state is
> updated in that state, since it will ignore the CRTCs that are kept running
> unmodified, and will thus unassign its associated FIFO, and later disable
> it.
>
> In order to make the code work as expected, let's pull the CRTC state of
> all the enabled CRTC in our atomic_check so that we can operate on all the
> running CRTCs, no matter whether they are affected by the new state or not.
>
> Cc: Hoegeun Kwon <hoegeun.kwon at samsung.com>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>

Thanks for the quick patch and detailed explanation.

Tested-by: Hoegeun Kwon <hoegeun.kwon at samsung.com>
Reviewed-by: Hoegeun Kwon <hoegeun.kwon at samsung.com>


Best regards,
Hoegeun

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 16e233e1406e..af3ee3dcdab6 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>   	struct drm_crtc *crtc;
>   	int i, ret;
>   
> +	/*
> +	 * Since the HVS FIFOs are shared across all the pixelvalves and
> +	 * the TXP (and thus all the CRTCs), we need to pull the current
> +	 * state of all the enabled CRTCs so that an update to a single
> +	 * CRTC still keeps the previous FIFOs enabled and assigned to
> +	 * the same CRTCs, instead of evaluating only the CRTC being
> +	 * modified.
> +	 */
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		if (!crtc->state->enable)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_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);