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

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

[Maxime Ripard]

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@samsung.com>
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 | 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);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@samsung.com>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Thanks for the quick patch and detailed explanation.

Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Reviewed-by: Hoegeun Kwon <hoegeun.kwon@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);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Stevenson]

Hi Maxime

On Thu, 17 Sep 2020 at 13:16, Maxime Ripard <maxime@cerno.tech> 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

s/contraints/constraints

> 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.

Yup, that would surely mess things up :)

> 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@samsung.com>
> 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>

Thanks
  Dave

> ---
>  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);
> --
> 2.26.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 879 bytes --]

On Fri, Sep 18, 2020 at 03:52:55PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Thu, 17 Sep 2020 at 13:16, Maxime Ripard <maxime@cerno.tech> 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
> 
> s/contraints/constraints

Oops, thanks

I've fixed it while applying it

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel