[PATCH 1/4] drm/vc4: kms: Split the HVS muxing check in a separate function

[PATCH 1/4] drm/vc4: kms: Split the HVS muxing check in a separate function

[Maxime Ripard]

The code that assigns HVS channels during atomic_check is starting to grow
a bit big, let's move it into a separate function.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 149825ff5df8..846fe8b3cb7a 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -612,13 +612,13 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 #define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
-static int
-vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
+static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
+				      struct drm_atomic_state *state)
 {
 	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
-	int i, ret;
+	unsigned int i;
 
 	/*
 	 * Since the HVS FIFOs are shared across all the pixelvalves and
@@ -691,6 +691,18 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 
+	return 0;
+}
+
+static int
+vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = vc4_pv_muxing_atomic_check(dev, state);
+	if (ret)
+		return ret;
+
 	ret = vc4_ctm_atomic_check(dev, state);
 	if (ret < 0)
 		return ret;
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard]

We've had a number of muxing corner-cases with specific ways to reproduce
them, so let's document them to make sure they aren't lost and introduce
regressions later on.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 846fe8b3cb7a..3b5e62842901 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -612,6 +612,28 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 #define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
+/*
+ * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
+ * the TXP (and therefore all the CRTCs found on that platform).
+ *
+ * The naive (and our initial) implementation would just iterate over
+ * all the active CRTCs, try to find a suitable FIFO, and then remove it
+ * from the available FIFOs pool. However, there's a few corner cases
+ * that need to be considered:
+ *
+ * - When running in a dual-display setup (so with two CRTCs involved),
+ *   we can update the state of a single CRTC (for example by changing
+ *   its mode using xrandr under X11) without affecting the other. In
+ *   this case, the other CRTC wouldn't be in the state at all, so we
+ *   need to consider all the running CRTCs in the DRM device to assign
+ *   a FIFO, not just the one in the state.
+ *
+ * - Since we need the pixelvalve to be disabled and enabled back when
+ *   the FIFO is changed, we should keep the FIFO assigned for as long
+ *   as the CRTC is enabled, only considering it free again once that
+ *   CRTC has been disabled. This can be tested by booting X11 on a
+ *   single display, and changing the resolution down and then back up.
+ */
 static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 				      struct drm_atomic_state *state)
 {
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Maxime Ripard]

The current HVS muxing code will consider the CRTCs in a given state to
setup their muxing in the HVS, and disable the other CRTCs muxes.

However, it's valid to only update a single CRTC with a state, and in this
situation we would mux out a CRTC that was enabled but left untouched by
the new state.

Fix this by considering all the CRTCs in the state and the ones with a
state and active.

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 | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 3b5e62842901..c9dfd5535a7e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -185,6 +185,23 @@ static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 	}
 }
 
+static struct drm_crtc_state *
+drm_atomic_get_new_or_current_crtc_state(struct drm_atomic_state *state,
+					 struct drm_crtc *crtc)
+{
+	struct drm_crtc_state *crtc_state;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	if (crtc_state)
+		return crtc_state;
+
+	return crtc->state;
+}
+
+#define for_each_new_or_current_crtc_state(__state, crtc, crtc_state)	\
+	list_for_each_entry(crtc, &__state->dev->mode_config.crtc_list, head) \
+		for_each_if(crtc_state = drm_atomic_get_new_or_current_crtc_state(__state, crtc))
+
 static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 				     struct drm_atomic_state *state)
 {
@@ -194,16 +211,16 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 	unsigned char dsp3_mux = 3;
 	unsigned char dsp4_mux = 3;
 	unsigned char dsp5_mux = 3;
-	unsigned int i;
 	u32 reg;
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
+	for_each_new_or_current_crtc_state(state, crtc, crtc_state) {
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+		struct vc4_crtc_state *vc4_state;
 
 		if (!crtc_state->active)
 			continue;
 
+		vc4_state = to_vc4_crtc_state(crtc_state);
 		switch (vc4_crtc->data->hvs_output) {
 		case 2:
 			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/vc4: kms: Fix VBLANK reporting on a disabled CRTC

[Maxime Ripard]

If a CRTC is enabled but not active, and that we're then doing a page flip
on another CRTC, drm_atomic_get_crtc_state will bring the first CRTC state
into the global state, and will make us wait for its vblank as well, even
though that might never occur.

Fix this by considering all the enabled CRTCs by either using their new
state in the global state, or using their current state if they aren't part
of the new state being checked, to remove their assigned channel from the
pool before started to assign channels to CRTCs enabled by the state.

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 | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index c9dfd5535a7e..0751ce5c6467 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -645,6 +645,14 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
  *   need to consider all the running CRTCs in the DRM device to assign
  *   a FIFO, not just the one in the state.
  *
+ * - To fix the above, we can't use drm_atomic_get_crtc_state on all
+ *   enabled CRTCs to pull their CRTC state into the global state, since
+ *   a page flip would start considering their vblank to complete. Since
+ *   we don't have a guarantee that they are actually active, that
+ *   vblank might never happen, and shouldn't even be considered if we
+ *   want to do a page flip on a single CRTC. That can be tested by
+ *   doing a modetest -v first on HDMI1 and then on HDMI0.
+ *
  * - Since we need the pixelvalve to be disabled and enabled back when
  *   the FIFO is changed, we should keep the FIFO assigned for as long
  *   as the CRTC is enabled, only considering it free again once that
@@ -656,6 +664,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 {
 	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
@@ -667,15 +676,14 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 	 * the same CRTCs, instead of evaluating only the CRTC being
 	 * modified.
 	 */
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		struct drm_crtc_state *crtc_state;
+	for_each_new_or_current_crtc_state(state, crtc, crtc_state) {
+		struct vc4_crtc_state *vc4_crtc_state;
 
-		if (!crtc->state->enable)
+		if (!crtc_state->enable)
 			continue;
 
-		crtc_state = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+		vc4_crtc_state = to_vc4_crtc_state(crtc_state);
+		unassigned_channels &= ~BIT(vc4_crtc_state->assigned_channel);
 	}
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -690,10 +698,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		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);
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
 			continue;
-		}
 
 		/*
 		 * The problem we have to solve here is that we have
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vc4: kms: Split the HVS muxing check in a separate function

[Hoegeun Kwon]

Hi Maxime,

On 10/8/20 8:25 PM, Maxime Ripard wrote:
> The code that assigns HVS channels during atomic_check is starting to grow
> a bit big, let's move it into a separate function.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Thanks for this patch set, I checked all patches well works.

All patches:

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

Best regards,
Hoegeun

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 149825ff5df8..846fe8b3cb7a 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -612,13 +612,13 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
>   #define NUM_OUTPUTS  6
>   #define NUM_CHANNELS 3
>   
> -static int
> -vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> +static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
> +				      struct drm_atomic_state *state)
>   {
>   	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>   	struct drm_crtc *crtc;
> -	int i, ret;
> +	unsigned int i;
>   
>   	/*
>   	 * Since the HVS FIFOs are shared across all the pixelvalves and
> @@ -691,6 +691,18 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>   		}
>   	}
>   
> +	return 0;
> +}
> +
> +static int
> +vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = vc4_pv_muxing_atomic_check(dev, state);
> +	if (ret)
> +		return ret;
> +
>   	ret = vc4_ctm_atomic_check(dev, state);
>   	if (ret < 0)
>   		return ret;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vc4: kms: Split the HVS muxing check in a separate function

[Maxime Ripard]

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

Hi Hoegeun,

On Mon, Oct 12, 2020 at 09:25:05PM +0900, Hoegeun Kwon wrote:
> Hi Maxime,
> 
> On 10/8/20 8:25 PM, Maxime Ripard wrote:
> > The code that assigns HVS channels during atomic_check is starting to grow
> > a bit big, let's move it into a separate function.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> Thanks for this patch set, I checked all patches well works.
> 
> All patches:
> 
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>

Following some discussion with Daniel last week, this is going to be
significantly reworked for the next iteration, so I won't take your tags
(but will Cc you for the next version).

Maxime

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel