[PATCH v2 0/7] drm/vc4: Rework the HVS muxing code

[PATCH v2 0/7] drm/vc4: Rework the HVS muxing code

[Maxime Ripard]

Hi,

Here's a second attempt at fixing the current issues we have with the
muxing code that results in a PV muxing its HVS muxing when only another
CRTC is modified by a state, or vblank timeouts when trying to wait for a
vblank on a single CRTC while another one is inactive but enabled.

Let me know what you think,
Maxime

Changes from v1:
  - Dropped the code trying to access all the CRTCs (whether in the state
    or not) state

Maxime Ripard (7):
  drm/vc4: kms: Remove useless define
  drm/vc4: kms: Rename NUM_CHANNELS
  drm/vc4: kms: Split the HVS muxing check in a separate function
  drm/vc4: kms: Document the muxing corner cases
  drm/vc4: kms: Add functions to create the state objects
  drm/vc4: kms: Store the unassigned channel list in the state
  drm/vc4: kms: Don't disable the muxing of an active CRTC

 drivers/gpu/drm/vc4/vc4_drv.h |   2 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 314 +++++++++++++++++++++++++----------
 2 files changed, 235 insertions(+), 81 deletions(-)

base-commit: 66e2a590a11f54865fb3ffd3ec932a6a4e477b40
-- 
git-series 0.9.1

[PATCH v2 0/7] drm/vc4: Rework the HVS muxing code

[Maxime Ripard]

Hi,

Here's a second attempt at fixing the current issues we have with the
muxing code that results in a PV muxing its HVS muxing when only another
CRTC is modified by a state, or vblank timeouts when trying to wait for a
vblank on a single CRTC while another one is inactive but enabled.

Let me know what you think,
Maxime

Changes from v1:
  - Dropped the code trying to access all the CRTCs (whether in the state
    or not) state

Maxime Ripard (7):
  drm/vc4: kms: Remove useless define
  drm/vc4: kms: Rename NUM_CHANNELS
  drm/vc4: kms: Split the HVS muxing check in a separate function
  drm/vc4: kms: Document the muxing corner cases
  drm/vc4: kms: Add functions to create the state objects
  drm/vc4: kms: Store the unassigned channel list in the state
  drm/vc4: kms: Don't disable the muxing of an active CRTC

 drivers/gpu/drm/vc4/vc4_drv.h |   2 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 314 +++++++++++++++++++++++++----------
 2 files changed, 235 insertions(+), 81 deletions(-)

base-commit: 66e2a590a11f54865fb3ffd3ec932a6a4e477b40
-- 
git-series 0.9.1

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

[PATCH v2 0/7] drm/vc4: Rework the HVS muxing code

[Maxime Ripard]

Hi,

Here's a second attempt at fixing the current issues we have with the
muxing code that results in a PV muxing its HVS muxing when only another
CRTC is modified by a state, or vblank timeouts when trying to wait for a
vblank on a single CRTC while another one is inactive but enabled.

Let me know what you think,
Maxime

Changes from v1:
  - Dropped the code trying to access all the CRTCs (whether in the state
    or not) state

Maxime Ripard (7):
  drm/vc4: kms: Remove useless define
  drm/vc4: kms: Rename NUM_CHANNELS
  drm/vc4: kms: Split the HVS muxing check in a separate function
  drm/vc4: kms: Document the muxing corner cases
  drm/vc4: kms: Add functions to create the state objects
  drm/vc4: kms: Store the unassigned channel list in the state
  drm/vc4: kms: Don't disable the muxing of an active CRTC

 drivers/gpu/drm/vc4/vc4_drv.h |   2 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 314 +++++++++++++++++++++++++----------
 2 files changed, 235 insertions(+), 81 deletions(-)

base-commit: 66e2a590a11f54865fb3ffd3ec932a6a4e477b40
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/7] drm/vc4: kms: Remove useless define

[Maxime Ripard]

NUM_OUTPUTS isn't used anymore, let's remove it.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 149825ff5df8..f8081c996193 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -609,7 +609,6 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
 static int
-- 
git-series 0.9.1

[PATCH v2 1/7] drm/vc4: kms: Remove useless define

[Maxime Ripard]

NUM_OUTPUTS isn't used anymore, let's remove it.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 149825ff5df8..f8081c996193 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -609,7 +609,6 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
 static int
-- 
git-series 0.9.1

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

[PATCH v2 1/7] drm/vc4: kms: Remove useless define

[Maxime Ripard]

NUM_OUTPUTS isn't used anymore, let's remove it.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 149825ff5df8..f8081c996193 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -609,7 +609,6 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
 static int
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/7] drm/vc4: kms: Rename NUM_CHANNELS

[Maxime Ripard]

The NUM_CHANNELS define has a pretty generic name and was right before the
function using it. Let's move to something that makes the hardware-specific
nature more obvious, and to a more appropriate place.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f8081c996193..80378c74fcd6 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -24,6 +24,8 @@
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
+#define HVS_NUM_CHANNELS 3
+
 struct vc4_ctm_state {
 	struct drm_private_state base;
 	struct drm_color_ctm *ctm;
@@ -609,12 +611,11 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_CHANNELS 3
 
 static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
+	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i, ret;
-- 
git-series 0.9.1

[PATCH v2 2/7] drm/vc4: kms: Rename NUM_CHANNELS

[Maxime Ripard]

The NUM_CHANNELS define has a pretty generic name and was right before the
function using it. Let's move to something that makes the hardware-specific
nature more obvious, and to a more appropriate place.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f8081c996193..80378c74fcd6 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -24,6 +24,8 @@
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
+#define HVS_NUM_CHANNELS 3
+
 struct vc4_ctm_state {
 	struct drm_private_state base;
 	struct drm_color_ctm *ctm;
@@ -609,12 +611,11 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_CHANNELS 3
 
 static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
+	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i, ret;
-- 
git-series 0.9.1

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

[PATCH v2 2/7] drm/vc4: kms: Rename NUM_CHANNELS

[Maxime Ripard]

The NUM_CHANNELS define has a pretty generic name and was right before the
function using it. Let's move to something that makes the hardware-specific
nature more obvious, and to a more appropriate place.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f8081c996193..80378c74fcd6 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -24,6 +24,8 @@
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
+#define HVS_NUM_CHANNELS 3
+
 struct vc4_ctm_state {
 	struct drm_private_state base;
 	struct drm_color_ctm *ctm;
@@ -609,12 +611,11 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
-#define NUM_CHANNELS 3
 
 static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
+	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i, ret;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/7] 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 80378c74fcd6..4aa0577bd055 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 = {
 };
 
 
-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(HVS_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;
-- 
git-series 0.9.1

[PATCH v2 3/7] 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 80378c74fcd6..4aa0577bd055 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 = {
 };
 
 
-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(HVS_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;
-- 
git-series 0.9.1

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

[PATCH v2 3/7] 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 80378c74fcd6..4aa0577bd055 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 = {
 };
 
 
-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(HVS_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;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/7] 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 4aa0577bd055..b0043abec16d 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 = {
 };
 
 
+/*
+ * 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)
 {
-- 
git-series 0.9.1

[PATCH v2 4/7] 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 4aa0577bd055..b0043abec16d 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 = {
 };
 
 
+/*
+ * 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)
 {
-- 
git-series 0.9.1

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

[PATCH v2 4/7] 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 4aa0577bd055..b0043abec16d 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 = {
 };
 
 
+/*
+ * 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)
 {
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/7] drm/vc4: kms: Add functions to create the state objects

[Maxime Ripard]

We're going to add a new private state, so let's move the previous state
function creation to some functions to make further additions easier.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index b0043abec16d..2cac556f7799 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -95,6 +95,27 @@ static const struct drm_private_state_funcs vc4_ctm_state_funcs = {
 	.atomic_destroy_state = vc4_ctm_destroy_state,
 };
 
+static int vc4_ctm_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_ctm_state *ctm_state;
+
+	drm_modeset_lock_init(&vc4->ctm_state_lock);
+
+	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
+	if (!ctm_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->ctm_manager, &ctm_state->base,
+				    &vc4_ctm_state_funcs);
+
+	return 0;
+}
+
+static void vc4_ctm_obj_fini(struct vc4_dev *vc4)
+{
+	drm_atomic_private_obj_fini(&vc4->ctm_manager);
+}
+
 /* Converts a DRM S31.32 value to the HW S0.9 format. */
 static u16 vc4_ctm_s31_32_to_s0_9(u64 in)
 {
@@ -611,6 +632,23 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
+static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_load_tracker_state *load_state;
+
+	if (!vc4->load_tracker_available)
+		return 0;
+
+	load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
+	if (!load_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->load_tracker,
+				    &load_state->base,
+				    &vc4_load_tracker_state_funcs);
+
+	return 0;
+}
 
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
@@ -745,8 +783,6 @@ static const struct drm_mode_config_funcs vc4_mode_funcs = {
 int vc4_kms_load(struct drm_device *dev)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	struct vc4_ctm_state *ctm_state;
-	struct vc4_load_tracker_state *load_state;
 	bool is_vc5 = of_device_is_compatible(dev->dev->of_node,
 					      "brcm,bcm2711-vc5");
 	int ret;
@@ -785,30 +821,22 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.async_page_flip = true;
 	dev->mode_config.allow_fb_modifiers = true;
 
-	drm_modeset_lock_init(&vc4->ctm_state_lock);
-
-	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
-	if (!ctm_state)
-		return -ENOMEM;
-
-	drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
-				    &vc4_ctm_state_funcs);
-
-	if (vc4->load_tracker_available) {
-		load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
-		if (!load_state) {
-			drm_atomic_private_obj_fini(&vc4->ctm_manager);
-			return -ENOMEM;
-		}
+	ret = vc4_ctm_obj_init(vc4);
+	if (ret)
+		return ret;
 
-		drm_atomic_private_obj_init(dev, &vc4->load_tracker,
-					    &load_state->base,
-					    &vc4_load_tracker_state_funcs);
-	}
+	ret = vc4_load_tracker_obj_init(vc4);
+	if (ret)
+		goto ctm_fini;
 
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
+
+ctm_fini:
+	vc4_ctm_obj_fini(vc4);
+
+	return ret;
 }
-- 
git-series 0.9.1

[PATCH v2 5/7] drm/vc4: kms: Add functions to create the state objects

[Maxime Ripard]

We're going to add a new private state, so let's move the previous state
function creation to some functions to make further additions easier.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index b0043abec16d..2cac556f7799 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -95,6 +95,27 @@ static const struct drm_private_state_funcs vc4_ctm_state_funcs = {
 	.atomic_destroy_state = vc4_ctm_destroy_state,
 };
 
+static int vc4_ctm_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_ctm_state *ctm_state;
+
+	drm_modeset_lock_init(&vc4->ctm_state_lock);
+
+	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
+	if (!ctm_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->ctm_manager, &ctm_state->base,
+				    &vc4_ctm_state_funcs);
+
+	return 0;
+}
+
+static void vc4_ctm_obj_fini(struct vc4_dev *vc4)
+{
+	drm_atomic_private_obj_fini(&vc4->ctm_manager);
+}
+
 /* Converts a DRM S31.32 value to the HW S0.9 format. */
 static u16 vc4_ctm_s31_32_to_s0_9(u64 in)
 {
@@ -611,6 +632,23 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
+static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_load_tracker_state *load_state;
+
+	if (!vc4->load_tracker_available)
+		return 0;
+
+	load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
+	if (!load_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->load_tracker,
+				    &load_state->base,
+				    &vc4_load_tracker_state_funcs);
+
+	return 0;
+}
 
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
@@ -745,8 +783,6 @@ static const struct drm_mode_config_funcs vc4_mode_funcs = {
 int vc4_kms_load(struct drm_device *dev)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	struct vc4_ctm_state *ctm_state;
-	struct vc4_load_tracker_state *load_state;
 	bool is_vc5 = of_device_is_compatible(dev->dev->of_node,
 					      "brcm,bcm2711-vc5");
 	int ret;
@@ -785,30 +821,22 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.async_page_flip = true;
 	dev->mode_config.allow_fb_modifiers = true;
 
-	drm_modeset_lock_init(&vc4->ctm_state_lock);
-
-	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
-	if (!ctm_state)
-		return -ENOMEM;
-
-	drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
-				    &vc4_ctm_state_funcs);
-
-	if (vc4->load_tracker_available) {
-		load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
-		if (!load_state) {
-			drm_atomic_private_obj_fini(&vc4->ctm_manager);
-			return -ENOMEM;
-		}
+	ret = vc4_ctm_obj_init(vc4);
+	if (ret)
+		return ret;
 
-		drm_atomic_private_obj_init(dev, &vc4->load_tracker,
-					    &load_state->base,
-					    &vc4_load_tracker_state_funcs);
-	}
+	ret = vc4_load_tracker_obj_init(vc4);
+	if (ret)
+		goto ctm_fini;
 
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
+
+ctm_fini:
+	vc4_ctm_obj_fini(vc4);
+
+	return ret;
 }
-- 
git-series 0.9.1

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

[PATCH v2 5/7] drm/vc4: kms: Add functions to create the state objects

[Maxime Ripard]

We're going to add a new private state, so let's move the previous state
function creation to some functions to make further additions easier.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index b0043abec16d..2cac556f7799 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -95,6 +95,27 @@ static const struct drm_private_state_funcs vc4_ctm_state_funcs = {
 	.atomic_destroy_state = vc4_ctm_destroy_state,
 };
 
+static int vc4_ctm_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_ctm_state *ctm_state;
+
+	drm_modeset_lock_init(&vc4->ctm_state_lock);
+
+	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
+	if (!ctm_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->ctm_manager, &ctm_state->base,
+				    &vc4_ctm_state_funcs);
+
+	return 0;
+}
+
+static void vc4_ctm_obj_fini(struct vc4_dev *vc4)
+{
+	drm_atomic_private_obj_fini(&vc4->ctm_manager);
+}
+
 /* Converts a DRM S31.32 value to the HW S0.9 format. */
 static u16 vc4_ctm_s31_32_to_s0_9(u64 in)
 {
@@ -611,6 +632,23 @@ static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
 	.atomic_destroy_state = vc4_load_tracker_destroy_state,
 };
 
+static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_load_tracker_state *load_state;
+
+	if (!vc4->load_tracker_available)
+		return 0;
+
+	load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
+	if (!load_state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(vc4->dev, &vc4->load_tracker,
+				    &load_state->base,
+				    &vc4_load_tracker_state_funcs);
+
+	return 0;
+}
 
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
@@ -745,8 +783,6 @@ static const struct drm_mode_config_funcs vc4_mode_funcs = {
 int vc4_kms_load(struct drm_device *dev)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	struct vc4_ctm_state *ctm_state;
-	struct vc4_load_tracker_state *load_state;
 	bool is_vc5 = of_device_is_compatible(dev->dev->of_node,
 					      "brcm,bcm2711-vc5");
 	int ret;
@@ -785,30 +821,22 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.async_page_flip = true;
 	dev->mode_config.allow_fb_modifiers = true;
 
-	drm_modeset_lock_init(&vc4->ctm_state_lock);
-
-	ctm_state = kzalloc(sizeof(*ctm_state), GFP_KERNEL);
-	if (!ctm_state)
-		return -ENOMEM;
-
-	drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
-				    &vc4_ctm_state_funcs);
-
-	if (vc4->load_tracker_available) {
-		load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
-		if (!load_state) {
-			drm_atomic_private_obj_fini(&vc4->ctm_manager);
-			return -ENOMEM;
-		}
+	ret = vc4_ctm_obj_init(vc4);
+	if (ret)
+		return ret;
 
-		drm_atomic_private_obj_init(dev, &vc4->load_tracker,
-					    &load_state->base,
-					    &vc4_load_tracker_state_funcs);
-	}
+	ret = vc4_load_tracker_obj_init(vc4);
+	if (ret)
+		goto ctm_fini;
 
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
+
+ctm_fini:
+	vc4_ctm_obj_fini(vc4);
+
+	return ret;
 }
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/7] drm/vc4: kms: Store the unassigned channel list in the state

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

Instead of creating the list of the free channels each time atomic_check
is called, and calling drm_atomic_get_crtc_state to retrieve the
allocated channels, let's create a private state object in the main
atomic state, and use it to store the available channels.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |   1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 126 ++++++++++++++++++++++++++++-------
 2 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 836fdca5e643..c6208b040f77 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -218,6 +218,7 @@ struct vc4_dev {
 
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
+	struct drm_private_obj hvs_channels;
 	struct drm_private_obj load_tracker;
 
 	/* List of vc4_debugfs_info_entry for adding to debugfs once
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2cac556f7799..2aa726b7422c 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -37,6 +37,17 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 	return container_of(priv, struct vc4_ctm_state, base);
 }
 
+struct vc4_hvs_state {
+	struct drm_private_state base;
+	unsigned int unassigned_channels;
+};
+
+static struct vc4_hvs_state *
+to_vc4_hvs_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct vc4_hvs_state, base);
+}
+
 struct vc4_load_tracker_state {
 	struct drm_private_state base;
 	u64 hvs_load;
@@ -650,6 +661,71 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return 0;
 }
 
+static void vc4_load_tracker_obj_fini(struct vc4_dev *vc4)
+{
+	if (!vc4->load_tracker_available)
+		return;
+
+	drm_atomic_private_obj_fini(&vc4->load_tracker);
+}
+
+static struct drm_private_state *
+vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
+{
+	struct vc4_hvs_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
+					   struct drm_private_state *state)
+{
+	struct vc4_hvs_state *hvs_state;
+
+	hvs_state = to_vc4_hvs_state(state);
+	kfree(hvs_state);
+}
+
+static const struct drm_private_state_funcs vc4_hvs_state_funcs = {
+	.atomic_duplicate_state = vc4_hvs_channels_duplicate_state,
+	.atomic_destroy_state = vc4_hvs_channels_destroy_state,
+};
+
+static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_hvs_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	drm_atomic_private_obj_init(vc4->dev, &vc4->hvs_channels,
+				    &state->base,
+				    &vc4_hvs_state_funcs);
+
+	return 0;
+}
+
+static struct vc4_hvs_state *
+vc4_hvs_get_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
  * the TXP (and therefore all the CRTCs found on that platform).
@@ -666,6 +742,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
  *   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
@@ -675,29 +759,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 				      struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	struct vc4_hvs_state *hvs_state;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
-	/*
-	 * 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) {
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc->state->enable)
-			continue;
-
-		crtc_state = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	}
+	hvs_state = vc4_hvs_get_global_state(state);
+	if (!hvs_state)
+		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct vc4_crtc_state *new_vc4_crtc_state =
@@ -705,16 +774,16 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
-		if (old_crtc_state->enable && !new_crtc_state->enable)
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
 			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);
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
 			continue;
-		}
 
 		/*
 		 * The problem we have to solve here is that we have
@@ -740,12 +809,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		 * the future, we will need to have something smarter,
 		 * but it works so far.
 		 */
-		matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
+		matching_channels = hvs_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
 		if (matching_channels) {
 			unsigned int channel = ffs(matching_channels) - 1;
 
 			new_vc4_crtc_state->assigned_channel = channel;
-			unassigned_channels &= ~BIT(channel);
+			hvs_state->unassigned_channels &= ~BIT(channel);
 		} else {
 			return -EINVAL;
 		}
@@ -829,12 +898,19 @@ int vc4_kms_load(struct drm_device *dev)
 	if (ret)
 		goto ctm_fini;
 
+	ret = vc4_hvs_channels_obj_init(vc4);
+	if (ret)
+		goto load_tracker_fini;
+
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
 
+load_tracker_fini:
+	vc4_load_tracker_obj_fini(vc4);
+
 ctm_fini:
 	vc4_ctm_obj_fini(vc4);
 
-- 
git-series 0.9.1

[PATCH v2 6/7] drm/vc4: kms: Store the unassigned channel list in the state

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

Instead of creating the list of the free channels each time atomic_check
is called, and calling drm_atomic_get_crtc_state to retrieve the
allocated channels, let's create a private state object in the main
atomic state, and use it to store the available channels.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |   1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 126 ++++++++++++++++++++++++++++-------
 2 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 836fdca5e643..c6208b040f77 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -218,6 +218,7 @@ struct vc4_dev {
 
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
+	struct drm_private_obj hvs_channels;
 	struct drm_private_obj load_tracker;
 
 	/* List of vc4_debugfs_info_entry for adding to debugfs once
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2cac556f7799..2aa726b7422c 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -37,6 +37,17 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 	return container_of(priv, struct vc4_ctm_state, base);
 }
 
+struct vc4_hvs_state {
+	struct drm_private_state base;
+	unsigned int unassigned_channels;
+};
+
+static struct vc4_hvs_state *
+to_vc4_hvs_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct vc4_hvs_state, base);
+}
+
 struct vc4_load_tracker_state {
 	struct drm_private_state base;
 	u64 hvs_load;
@@ -650,6 +661,71 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return 0;
 }
 
+static void vc4_load_tracker_obj_fini(struct vc4_dev *vc4)
+{
+	if (!vc4->load_tracker_available)
+		return;
+
+	drm_atomic_private_obj_fini(&vc4->load_tracker);
+}
+
+static struct drm_private_state *
+vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
+{
+	struct vc4_hvs_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
+					   struct drm_private_state *state)
+{
+	struct vc4_hvs_state *hvs_state;
+
+	hvs_state = to_vc4_hvs_state(state);
+	kfree(hvs_state);
+}
+
+static const struct drm_private_state_funcs vc4_hvs_state_funcs = {
+	.atomic_duplicate_state = vc4_hvs_channels_duplicate_state,
+	.atomic_destroy_state = vc4_hvs_channels_destroy_state,
+};
+
+static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_hvs_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	drm_atomic_private_obj_init(vc4->dev, &vc4->hvs_channels,
+				    &state->base,
+				    &vc4_hvs_state_funcs);
+
+	return 0;
+}
+
+static struct vc4_hvs_state *
+vc4_hvs_get_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
  * the TXP (and therefore all the CRTCs found on that platform).
@@ -666,6 +742,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
  *   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
@@ -675,29 +759,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 				      struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	struct vc4_hvs_state *hvs_state;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
-	/*
-	 * 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) {
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc->state->enable)
-			continue;
-
-		crtc_state = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	}
+	hvs_state = vc4_hvs_get_global_state(state);
+	if (!hvs_state)
+		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct vc4_crtc_state *new_vc4_crtc_state =
@@ -705,16 +774,16 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
-		if (old_crtc_state->enable && !new_crtc_state->enable)
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
 			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);
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
 			continue;
-		}
 
 		/*
 		 * The problem we have to solve here is that we have
@@ -740,12 +809,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		 * the future, we will need to have something smarter,
 		 * but it works so far.
 		 */
-		matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
+		matching_channels = hvs_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
 		if (matching_channels) {
 			unsigned int channel = ffs(matching_channels) - 1;
 
 			new_vc4_crtc_state->assigned_channel = channel;
-			unassigned_channels &= ~BIT(channel);
+			hvs_state->unassigned_channels &= ~BIT(channel);
 		} else {
 			return -EINVAL;
 		}
@@ -829,12 +898,19 @@ int vc4_kms_load(struct drm_device *dev)
 	if (ret)
 		goto ctm_fini;
 
+	ret = vc4_hvs_channels_obj_init(vc4);
+	if (ret)
+		goto load_tracker_fini;
+
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
 
+load_tracker_fini:
+	vc4_load_tracker_obj_fini(vc4);
+
 ctm_fini:
 	vc4_ctm_obj_fini(vc4);
 
-- 
git-series 0.9.1

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

[PATCH v2 6/7] drm/vc4: kms: Store the unassigned channel list in the state

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

Instead of creating the list of the free channels each time atomic_check
is called, and calling drm_atomic_get_crtc_state to retrieve the
allocated channels, let's create a private state object in the main
atomic state, and use it to store the available channels.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |   1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 126 ++++++++++++++++++++++++++++-------
 2 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 836fdca5e643..c6208b040f77 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -218,6 +218,7 @@ struct vc4_dev {
 
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
+	struct drm_private_obj hvs_channels;
 	struct drm_private_obj load_tracker;
 
 	/* List of vc4_debugfs_info_entry for adding to debugfs once
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2cac556f7799..2aa726b7422c 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -37,6 +37,17 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 	return container_of(priv, struct vc4_ctm_state, base);
 }
 
+struct vc4_hvs_state {
+	struct drm_private_state base;
+	unsigned int unassigned_channels;
+};
+
+static struct vc4_hvs_state *
+to_vc4_hvs_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct vc4_hvs_state, base);
+}
+
 struct vc4_load_tracker_state {
 	struct drm_private_state base;
 	u64 hvs_load;
@@ -650,6 +661,71 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return 0;
 }
 
+static void vc4_load_tracker_obj_fini(struct vc4_dev *vc4)
+{
+	if (!vc4->load_tracker_available)
+		return;
+
+	drm_atomic_private_obj_fini(&vc4->load_tracker);
+}
+
+static struct drm_private_state *
+vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
+{
+	struct vc4_hvs_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
+					   struct drm_private_state *state)
+{
+	struct vc4_hvs_state *hvs_state;
+
+	hvs_state = to_vc4_hvs_state(state);
+	kfree(hvs_state);
+}
+
+static const struct drm_private_state_funcs vc4_hvs_state_funcs = {
+	.atomic_duplicate_state = vc4_hvs_channels_duplicate_state,
+	.atomic_destroy_state = vc4_hvs_channels_destroy_state,
+};
+
+static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
+{
+	struct vc4_hvs_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	drm_atomic_private_obj_init(vc4->dev, &vc4->hvs_channels,
+				    &state->base,
+				    &vc4_hvs_state_funcs);
+
+	return 0;
+}
+
+static struct vc4_hvs_state *
+vc4_hvs_get_global_state(struct drm_atomic_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 /*
  * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
  * the TXP (and therefore all the CRTCs found on that platform).
@@ -666,6 +742,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
  *   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
@@ -675,29 +759,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 				      struct drm_atomic_state *state)
 {
-	unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
+	struct vc4_hvs_state *hvs_state;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
-	/*
-	 * 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) {
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc->state->enable)
-			continue;
-
-		crtc_state = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	}
+	hvs_state = vc4_hvs_get_global_state(state);
+	if (!hvs_state)
+		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct vc4_crtc_state *new_vc4_crtc_state =
@@ -705,16 +774,16 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
-		if (old_crtc_state->enable && !new_crtc_state->enable)
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
 			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);
+		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
 			continue;
-		}
 
 		/*
 		 * The problem we have to solve here is that we have
@@ -740,12 +809,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		 * the future, we will need to have something smarter,
 		 * but it works so far.
 		 */
-		matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
+		matching_channels = hvs_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
 		if (matching_channels) {
 			unsigned int channel = ffs(matching_channels) - 1;
 
 			new_vc4_crtc_state->assigned_channel = channel;
-			unassigned_channels &= ~BIT(channel);
+			hvs_state->unassigned_channels &= ~BIT(channel);
 		} else {
 			return -EINVAL;
 		}
@@ -829,12 +898,19 @@ int vc4_kms_load(struct drm_device *dev)
 	if (ret)
 		goto ctm_fini;
 
+	ret = vc4_hvs_channels_obj_init(vc4);
+	if (ret)
+		goto load_tracker_fini;
+
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
 
+load_tracker_fini:
+	vc4_load_tracker_obj_fini(vc4);
+
 ctm_fini:
 	vc4_ctm_obj_fini(vc4);
 
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 7/7] 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 setting a flag on the CRTC state when the muxing has been
changed, and only change the muxing configuration when that flag is there.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index c6208b040f77..c074c0538e57 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -523,6 +523,7 @@ struct vc4_crtc_state {
 	struct drm_mm_node mm;
 	bool feed_txp;
 	bool txp_armed;
+	bool needs_muxing;
 	unsigned int assigned_channel;
 
 	struct {
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2aa726b7422c..409aeb19d210 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
-	unsigned char dsp2_mux = 0;
-	unsigned char dsp3_mux = 3;
-	unsigned char dsp4_mux = 3;
-	unsigned char dsp5_mux = 3;
+	unsigned char mux;
 	unsigned int i;
 	u32 reg;
 
@@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 
-		if (!crtc_state->active)
+		if (!vc4_state->needs_muxing)
 			continue;
 
 		switch (vc4_crtc->data->hvs_output) {
 		case 2:
-			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			reg = HVS_READ(SCALER_DISPECTRL);
+			HVS_WRITE(SCALER_DISPECTRL,
+				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
 			break;
 
 		case 3:
-			dsp3_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPCTRL);
+			HVS_WRITE(SCALER_DISPCTRL,
+				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
 			break;
 
 		case 4:
-			dsp4_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPEOLN);
+			HVS_WRITE(SCALER_DISPEOLN,
+				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
+
 			break;
 
 		case 5:
-			dsp5_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPDITHER);
+			HVS_WRITE(SCALER_DISPDITHER,
+				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
 			break;
 
 		default:
 			break;
 		}
 	}
-
-	reg = HVS_READ(SCALER_DISPECTRL);
-	HVS_WRITE(SCALER_DISPECTRL,
-		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
-		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
-
-	reg = HVS_READ(SCALER_DISPCTRL);
-	HVS_WRITE(SCALER_DISPCTRL,
-		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
-		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
-
-	reg = HVS_READ(SCALER_DISPEOLN);
-	HVS_WRITE(SCALER_DISPEOLN,
-		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
-		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
-
-	reg = HVS_READ(SCALER_DISPDITHER);
-	HVS_WRITE(SCALER_DISPDITHER,
-		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
-		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
 }
 
 static void
@@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct vc4_crtc_state *old_vc4_crtc_state =
+			to_vc4_crtc_state(old_crtc_state);
 		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 (old_crtc_state->enable && !new_crtc_state->enable) {
-			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
-			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
+		/* Nothing to do here, let's skip it */
+		if ((old_crtc_state->enable && new_crtc_state->enable) ||
+		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
+			new_vc4_crtc_state->needs_muxing = false;
+			continue;
 		}
 
-		if (!new_crtc_state->enable)
-			continue;
+		/* Muxing will need to be modified, mark it as such */
+		new_vc4_crtc_state->needs_muxing = true;
 
-		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
+		/* If we're disabling our CRTC, we put back our channel */
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
+			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
+		}
 
 		/*
 		 * The problem we have to solve here is that we have
-- 
git-series 0.9.1

[PATCH v2 7/7] 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 setting a flag on the CRTC state when the muxing has been
changed, and only change the muxing configuration when that flag is there.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index c6208b040f77..c074c0538e57 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -523,6 +523,7 @@ struct vc4_crtc_state {
 	struct drm_mm_node mm;
 	bool feed_txp;
 	bool txp_armed;
+	bool needs_muxing;
 	unsigned int assigned_channel;
 
 	struct {
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2aa726b7422c..409aeb19d210 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
-	unsigned char dsp2_mux = 0;
-	unsigned char dsp3_mux = 3;
-	unsigned char dsp4_mux = 3;
-	unsigned char dsp5_mux = 3;
+	unsigned char mux;
 	unsigned int i;
 	u32 reg;
 
@@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 
-		if (!crtc_state->active)
+		if (!vc4_state->needs_muxing)
 			continue;
 
 		switch (vc4_crtc->data->hvs_output) {
 		case 2:
-			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			reg = HVS_READ(SCALER_DISPECTRL);
+			HVS_WRITE(SCALER_DISPECTRL,
+				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
 			break;
 
 		case 3:
-			dsp3_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPCTRL);
+			HVS_WRITE(SCALER_DISPCTRL,
+				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
 			break;
 
 		case 4:
-			dsp4_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPEOLN);
+			HVS_WRITE(SCALER_DISPEOLN,
+				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
+
 			break;
 
 		case 5:
-			dsp5_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPDITHER);
+			HVS_WRITE(SCALER_DISPDITHER,
+				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
 			break;
 
 		default:
 			break;
 		}
 	}
-
-	reg = HVS_READ(SCALER_DISPECTRL);
-	HVS_WRITE(SCALER_DISPECTRL,
-		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
-		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
-
-	reg = HVS_READ(SCALER_DISPCTRL);
-	HVS_WRITE(SCALER_DISPCTRL,
-		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
-		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
-
-	reg = HVS_READ(SCALER_DISPEOLN);
-	HVS_WRITE(SCALER_DISPEOLN,
-		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
-		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
-
-	reg = HVS_READ(SCALER_DISPDITHER);
-	HVS_WRITE(SCALER_DISPDITHER,
-		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
-		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
 }
 
 static void
@@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct vc4_crtc_state *old_vc4_crtc_state =
+			to_vc4_crtc_state(old_crtc_state);
 		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 (old_crtc_state->enable && !new_crtc_state->enable) {
-			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
-			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
+		/* Nothing to do here, let's skip it */
+		if ((old_crtc_state->enable && new_crtc_state->enable) ||
+		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
+			new_vc4_crtc_state->needs_muxing = false;
+			continue;
 		}
 
-		if (!new_crtc_state->enable)
-			continue;
+		/* Muxing will need to be modified, mark it as such */
+		new_vc4_crtc_state->needs_muxing = true;
 
-		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
+		/* If we're disabling our CRTC, we put back our channel */
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
+			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
+		}
 
 		/*
 		 * The problem we have to solve here is that we have
-- 
git-series 0.9.1

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

[PATCH v2 7/7] 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 setting a flag on the CRTC state when the muxing has been
changed, and only change the muxing configuration when that flag is there.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
 drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index c6208b040f77..c074c0538e57 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -523,6 +523,7 @@ struct vc4_crtc_state {
 	struct drm_mm_node mm;
 	bool feed_txp;
 	bool txp_armed;
+	bool needs_muxing;
 	unsigned int assigned_channel;
 
 	struct {
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2aa726b7422c..409aeb19d210 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
-	unsigned char dsp2_mux = 0;
-	unsigned char dsp3_mux = 3;
-	unsigned char dsp4_mux = 3;
-	unsigned char dsp5_mux = 3;
+	unsigned char mux;
 	unsigned int i;
 	u32 reg;
 
@@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 
-		if (!crtc_state->active)
+		if (!vc4_state->needs_muxing)
 			continue;
 
 		switch (vc4_crtc->data->hvs_output) {
 		case 2:
-			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
+			reg = HVS_READ(SCALER_DISPECTRL);
+			HVS_WRITE(SCALER_DISPECTRL,
+				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
 			break;
 
 		case 3:
-			dsp3_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPCTRL);
+			HVS_WRITE(SCALER_DISPCTRL,
+				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
 			break;
 
 		case 4:
-			dsp4_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPEOLN);
+			HVS_WRITE(SCALER_DISPEOLN,
+				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
+
 			break;
 
 		case 5:
-			dsp5_mux = vc4_state->assigned_channel;
+			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
+				mux = 3;
+			else
+				mux = vc4_state->assigned_channel;
+
+			reg = HVS_READ(SCALER_DISPDITHER);
+			HVS_WRITE(SCALER_DISPDITHER,
+				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
+				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
 			break;
 
 		default:
 			break;
 		}
 	}
-
-	reg = HVS_READ(SCALER_DISPECTRL);
-	HVS_WRITE(SCALER_DISPECTRL,
-		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
-		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
-
-	reg = HVS_READ(SCALER_DISPCTRL);
-	HVS_WRITE(SCALER_DISPCTRL,
-		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
-		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
-
-	reg = HVS_READ(SCALER_DISPEOLN);
-	HVS_WRITE(SCALER_DISPEOLN,
-		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
-		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
-
-	reg = HVS_READ(SCALER_DISPDITHER);
-	HVS_WRITE(SCALER_DISPDITHER,
-		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
-		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
 }
 
 static void
@@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		return -EINVAL;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		struct vc4_crtc_state *old_vc4_crtc_state =
+			to_vc4_crtc_state(old_crtc_state);
 		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 (old_crtc_state->enable && !new_crtc_state->enable) {
-			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
-			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
+		/* Nothing to do here, let's skip it */
+		if ((old_crtc_state->enable && new_crtc_state->enable) ||
+		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
+			new_vc4_crtc_state->needs_muxing = false;
+			continue;
 		}
 
-		if (!new_crtc_state->enable)
-			continue;
+		/* Muxing will need to be modified, mark it as such */
+		new_vc4_crtc_state->needs_muxing = true;
 
-		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
+		/* If we're disabling our CRTC, we put back our channel */
+		if (old_crtc_state->enable && !new_crtc_state->enable) {
+			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
+			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
+		}
 
 		/*
 		 * The problem we have to solve here is that we have
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> 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 4aa0577bd055..b0043abec16d 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 = {
>  };
>  
>  
> +/*
> + * 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.

This is a bit much. With planes we have the same problem, and we're
solving this with the drm_plane_state->commit tracker. If you have one of
these per fifo then you can easily sync against the disabling crtc if the
fifo becomes free.

Note to avoid locking headaches this would mean you'd need a per-fifo
private state object. You can avoid this if you just track the
->disabling_commit per fifo, and sync against that when enabling it on a
different fifo.

Note that it's somewhat tricky to do this correctly, since you need to
copy that commit on each state duplication around, until it's either used
in a new crtc (and hence tracked under that) or the commit has completed
(but this is only an optimization, you could just keep them around forever
for unused fifo that have been used in the past, it's a tiny struct with
nothing hanging of it).
-Daniel

> + */
>  static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  				      struct drm_atomic_state *state)
>  {
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> 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 4aa0577bd055..b0043abec16d 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 = {
>  };
>  
>  
> +/*
> + * 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.

This is a bit much. With planes we have the same problem, and we're
solving this with the drm_plane_state->commit tracker. If you have one of
these per fifo then you can easily sync against the disabling crtc if the
fifo becomes free.

Note to avoid locking headaches this would mean you'd need a per-fifo
private state object. You can avoid this if you just track the
->disabling_commit per fifo, and sync against that when enabling it on a
different fifo.

Note that it's somewhat tricky to do this correctly, since you need to
copy that commit on each state duplication around, until it's either used
in a new crtc (and hence tracked under that) or the commit has completed
(but this is only an optimization, you could just keep them around forever
for unused fifo that have been used in the past, it's a tiny struct with
nothing hanging of it).
-Daniel

> + */
>  static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  				      struct drm_atomic_state *state)
>  {
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> 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 4aa0577bd055..b0043abec16d 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 = {
>  };
>  
>  
> +/*
> + * 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.

This is a bit much. With planes we have the same problem, and we're
solving this with the drm_plane_state->commit tracker. If you have one of
these per fifo then you can easily sync against the disabling crtc if the
fifo becomes free.

Note to avoid locking headaches this would mean you'd need a per-fifo
private state object. You can avoid this if you just track the
->disabling_commit per fifo, and sync against that when enabling it on a
different fifo.

Note that it's somewhat tricky to do this correctly, since you need to
copy that commit on each state duplication around, until it's either used
in a new crtc (and hence tracked under that) or the commit has completed
(but this is only an optimization, you could just keep them around forever
for unused fifo that have been used in the past, it's a tiny struct with
nothing hanging of it).
-Daniel

> + */
>  static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  				      struct drm_atomic_state *state)
>  {
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]

Hi!

Thanks for your review

On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > 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 4aa0577bd055..b0043abec16d 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 = {
> >  };
> >  
> >  
> > +/*
> > + * 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.
> 
> This is a bit much.

What do you find to be a bit much?

> With planes we have the same problem, and we're solving this with the
> drm_plane_state->commit tracker. If you have one of these per fifo
> then you can easily sync against the disabling crtc if the fifo
> becomes free.
> 
> Note to avoid locking headaches this would mean you'd need a per-fifo
> private state object. You can avoid this if you just track the
> ->disabling_commit per fifo, and sync against that when enabling it on a
> different fifo.
> 
> Note that it's somewhat tricky to do this correctly, since you need to
> copy that commit on each state duplication around, until it's either used
> in a new crtc (and hence tracked under that) or the commit has completed
> (but this is only an optimization, you could just keep them around forever
> for unused fifo that have been used in the past, it's a tiny struct with
> nothing hanging of it).

I'm not really following you here. The hardware that does the blending
(HVS) and the timing generation (pixelvalve) is mostly transparent to
DRM and plugged as a CRTC, with the pixelvalve being the device tied to
that CRTC, and the pixelvalve hooks calling into the HVS code when
needed.

The FIFO is in the HVS itself since it can only blend 3 different
scenes, and then you get to choose the output of that FIFO to send it to
the proper pixelvalve that matches the encoder you eventually want to
use.

So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
concerned.

Maxime

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

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard]

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

Hi!

Thanks for your review

On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > 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 4aa0577bd055..b0043abec16d 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 = {
> >  };
> >  
> >  
> > +/*
> > + * 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.
> 
> This is a bit much.

What do you find to be a bit much?

> With planes we have the same problem, and we're solving this with the
> drm_plane_state->commit tracker. If you have one of these per fifo
> then you can easily sync against the disabling crtc if the fifo
> becomes free.
> 
> Note to avoid locking headaches this would mean you'd need a per-fifo
> private state object. You can avoid this if you just track the
> ->disabling_commit per fifo, and sync against that when enabling it on a
> different fifo.
> 
> Note that it's somewhat tricky to do this correctly, since you need to
> copy that commit on each state duplication around, until it's either used
> in a new crtc (and hence tracked under that) or the commit has completed
> (but this is only an optimization, you could just keep them around forever
> for unused fifo that have been used in the past, it's a tiny struct with
> nothing hanging of it).

I'm not really following you here. The hardware that does the blending
(HVS) and the timing generation (pixelvalve) is mostly transparent to
DRM and plugged as a CRTC, with the pixelvalve being the device tied to
that CRTC, and the pixelvalve hooks calling into the HVS code when
needed.

The FIFO is in the HVS itself since it can only blend 3 different
scenes, and then you get to choose the output of that FIFO to send it to
the proper pixelvalve that matches the encoder you eventually want to
use.

So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
concerned.

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

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard]

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

Hi!

Thanks for your review

On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > 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 4aa0577bd055..b0043abec16d 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 = {
> >  };
> >  
> >  
> > +/*
> > + * 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.
> 
> This is a bit much.

What do you find to be a bit much?

> With planes we have the same problem, and we're solving this with the
> drm_plane_state->commit tracker. If you have one of these per fifo
> then you can easily sync against the disabling crtc if the fifo
> becomes free.
> 
> Note to avoid locking headaches this would mean you'd need a per-fifo
> private state object. You can avoid this if you just track the
> ->disabling_commit per fifo, and sync against that when enabling it on a
> different fifo.
> 
> Note that it's somewhat tricky to do this correctly, since you need to
> copy that commit on each state duplication around, until it's either used
> in a new crtc (and hence tracked under that) or the commit has completed
> (but this is only an optimization, you could just keep them around forever
> for unused fifo that have been used in the past, it's a tiny struct with
> nothing hanging of it).

I'm not really following you here. The hardware that does the blending
(HVS) and the timing generation (pixelvalve) is mostly transparent to
DRM and plugged as a CRTC, with the pixelvalve being the device tied to
that CRTC, and the pixelvalve hooks calling into the HVS code when
needed.

The FIFO is in the HVS itself since it can only blend 3 different
scenes, and then you get to choose the output of that FIFO to send it to
the proper pixelvalve that matches the encoder you eventually want to
use.

So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
concerned.

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

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Thu, Oct 29, 2020 at 11:47:28AM +0100, Maxime Ripard wrote:
> Hi!
> 
> Thanks for your review
> 
> On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> > On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > > 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 4aa0577bd055..b0043abec16d 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 = {
> > >  };
> > >  
> > >  
> > > +/*
> > > + * 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.
> > 
> > This is a bit much.
> 
> What do you find to be a bit much?
> 
> > With planes we have the same problem, and we're solving this with the
> > drm_plane_state->commit tracker. If you have one of these per fifo
> > then you can easily sync against the disabling crtc if the fifo
> > becomes free.
> > 
> > Note to avoid locking headaches this would mean you'd need a per-fifo
> > private state object. You can avoid this if you just track the
> > ->disabling_commit per fifo, and sync against that when enabling it on a
> > different fifo.
> > 
> > Note that it's somewhat tricky to do this correctly, since you need to
> > copy that commit on each state duplication around, until it's either used
> > in a new crtc (and hence tracked under that) or the commit has completed
> > (but this is only an optimization, you could just keep them around forever
> > for unused fifo that have been used in the past, it's a tiny struct with
> > nothing hanging of it).
> 
> I'm not really following you here. The hardware that does the blending
> (HVS) and the timing generation (pixelvalve) is mostly transparent to
> DRM and plugged as a CRTC, with the pixelvalve being the device tied to
> that CRTC, and the pixelvalve hooks calling into the HVS code when
> needed.
> 
> The FIFO is in the HVS itself since it can only blend 3 different
> scenes, and then you get to choose the output of that FIFO to send it to
> the proper pixelvalve that matches the encoder you eventually want to
> use.
> 
> So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
> concerned.

So why do you dynamically assign it in a global state object? It sounded
like you assign these things dynamically, and that there's a problem with
sync when you move it from one crtc to the other. And that kind of problem
is solved by tracking the last crtc using a given resource that can be
used by different crtc with a drm_crtc_commit *last_user pointer.

Otherwise I think if you follow 2 crtc commits, one that disables a CRTC
user and releases a FIFO, and the next crtc (a different one) that uses it
right away, both nonblocking, then the 2nd crtc might start using your
shared resources before the first one actually stopped using it.

The other thing is also if you need multiple of these shared resources on
a CRTC, and dynamically reassigning them, then forcing them to be assigned
until the crtc is completely off is a bit too much synchronization. E.g.
we don't have that rule for drm planes. Now I have no idea whether your
CRTC:FIFO is 1:1 or 1:n, so maybe you only have the sync issue and not the
over-sync issue :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Thu, Oct 29, 2020 at 11:47:28AM +0100, Maxime Ripard wrote:
> Hi!
> 
> Thanks for your review
> 
> On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> > On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > > 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 4aa0577bd055..b0043abec16d 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 = {
> > >  };
> > >  
> > >  
> > > +/*
> > > + * 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.
> > 
> > This is a bit much.
> 
> What do you find to be a bit much?
> 
> > With planes we have the same problem, and we're solving this with the
> > drm_plane_state->commit tracker. If you have one of these per fifo
> > then you can easily sync against the disabling crtc if the fifo
> > becomes free.
> > 
> > Note to avoid locking headaches this would mean you'd need a per-fifo
> > private state object. You can avoid this if you just track the
> > ->disabling_commit per fifo, and sync against that when enabling it on a
> > different fifo.
> > 
> > Note that it's somewhat tricky to do this correctly, since you need to
> > copy that commit on each state duplication around, until it's either used
> > in a new crtc (and hence tracked under that) or the commit has completed
> > (but this is only an optimization, you could just keep them around forever
> > for unused fifo that have been used in the past, it's a tiny struct with
> > nothing hanging of it).
> 
> I'm not really following you here. The hardware that does the blending
> (HVS) and the timing generation (pixelvalve) is mostly transparent to
> DRM and plugged as a CRTC, with the pixelvalve being the device tied to
> that CRTC, and the pixelvalve hooks calling into the HVS code when
> needed.
> 
> The FIFO is in the HVS itself since it can only blend 3 different
> scenes, and then you get to choose the output of that FIFO to send it to
> the proper pixelvalve that matches the encoder you eventually want to
> use.
> 
> So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
> concerned.

So why do you dynamically assign it in a global state object? It sounded
like you assign these things dynamically, and that there's a problem with
sync when you move it from one crtc to the other. And that kind of problem
is solved by tracking the last crtc using a given resource that can be
used by different crtc with a drm_crtc_commit *last_user pointer.

Otherwise I think if you follow 2 crtc commits, one that disables a CRTC
user and releases a FIFO, and the next crtc (a different one) that uses it
right away, both nonblocking, then the 2nd crtc might start using your
shared resources before the first one actually stopped using it.

The other thing is also if you need multiple of these shared resources on
a CRTC, and dynamically reassigning them, then forcing them to be assigned
until the crtc is completely off is a bit too much synchronization. E.g.
we don't have that rule for drm planes. Now I have no idea whether your
CRTC:FIFO is 1:1 or 1:n, so maybe you only have the sync issue and not the
over-sync issue :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Re: [PATCH v2 4/7] drm/vc4: kms: Document the muxing corner cases

[Daniel Vetter]

On Thu, Oct 29, 2020 at 11:47:28AM +0100, Maxime Ripard wrote:
> Hi!
> 
> Thanks for your review
> 
> On Thu, Oct 29, 2020 at 09:51:04AM +0100, Daniel Vetter wrote:
> > On Wed, Oct 28, 2020 at 01:41:01PM +0100, Maxime Ripard wrote:
> > > 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 4aa0577bd055..b0043abec16d 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 = {
> > >  };
> > >  
> > >  
> > > +/*
> > > + * 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.
> > 
> > This is a bit much.
> 
> What do you find to be a bit much?
> 
> > With planes we have the same problem, and we're solving this with the
> > drm_plane_state->commit tracker. If you have one of these per fifo
> > then you can easily sync against the disabling crtc if the fifo
> > becomes free.
> > 
> > Note to avoid locking headaches this would mean you'd need a per-fifo
> > private state object. You can avoid this if you just track the
> > ->disabling_commit per fifo, and sync against that when enabling it on a
> > different fifo.
> > 
> > Note that it's somewhat tricky to do this correctly, since you need to
> > copy that commit on each state duplication around, until it's either used
> > in a new crtc (and hence tracked under that) or the commit has completed
> > (but this is only an optimization, you could just keep them around forever
> > for unused fifo that have been used in the past, it's a tiny struct with
> > nothing hanging of it).
> 
> I'm not really following you here. The hardware that does the blending
> (HVS) and the timing generation (pixelvalve) is mostly transparent to
> DRM and plugged as a CRTC, with the pixelvalve being the device tied to
> that CRTC, and the pixelvalve hooks calling into the HVS code when
> needed.
> 
> The FIFO is in the HVS itself since it can only blend 3 different
> scenes, and then you get to choose the output of that FIFO to send it to
> the proper pixelvalve that matches the encoder you eventually want to
> use.
> 
> So yeah, this FIFO is entirely internal to the CRTC as far as DRM is
> concerned.

So why do you dynamically assign it in a global state object? It sounded
like you assign these things dynamically, and that there's a problem with
sync when you move it from one crtc to the other. And that kind of problem
is solved by tracking the last crtc using a given resource that can be
used by different crtc with a drm_crtc_commit *last_user pointer.

Otherwise I think if you follow 2 crtc commits, one that disables a CRTC
user and releases a FIFO, and the next crtc (a different one) that uses it
right away, both nonblocking, then the 2nd crtc might start using your
shared resources before the first one actually stopped using it.

The other thing is also if you need multiple of these shared resources on
a CRTC, and dynamically reassigning them, then forcing them to be assigned
until the crtc is completely off is a bit too much synchronization. E.g.
we don't have that rule for drm planes. Now I have no idea whether your
CRTC:FIFO is 1:1 or 1:n, so maybe you only have the sync issue and not the
over-sync issue :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

Thanks for V2 patch.


On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);

In my opinion, the old_vc4_crtc_state definition is better to move to 
patch6.
Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.


Best regards,
Hoegeun

>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

Thanks for V2 patch.


On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);

In my opinion, the old_vc4_crtc_state definition is better to move to 
patch6.
Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.


Best regards,
Hoegeun

>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have

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

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

Thanks for V2 patch.


On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);

In my opinion, the old_vc4_crtc_state definition is better to move to 
patch6.
Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.


Best regards,
Hoegeun

>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

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_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);
>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

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_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);
>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have

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

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Hoegeun Kwon]

Hi Maxime,

On 10/28/20 9:41 PM, Maxime Ripard wrote:
> 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 setting a flag on the CRTC state when the muxing has been
> changed, and only change the muxing configuration when that flag is there.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

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_drv.h |  1 +-
>   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
>   2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c6208b040f77..c074c0538e57 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -523,6 +523,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;
>   	unsigned int assigned_channel;
>   
>   	struct {
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2aa726b7422c..409aeb19d210 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   {
>   	struct drm_crtc_state *crtc_state;
>   	struct drm_crtc *crtc;
> -	unsigned char dsp2_mux = 0;
> -	unsigned char dsp3_mux = 3;
> -	unsigned char dsp4_mux = 3;
> -	unsigned char dsp5_mux = 3;
> +	unsigned char mux;
>   	unsigned int i;
>   	u32 reg;
>   
> @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
>   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   
> -		if (!crtc_state->active)
> +		if (!vc4_state->needs_muxing)
>   			continue;
>   
>   		switch (vc4_crtc->data->hvs_output) {
>   		case 2:
> -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> +			reg = HVS_READ(SCALER_DISPECTRL);
> +			HVS_WRITE(SCALER_DISPECTRL,
> +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
>   			break;
>   
>   		case 3:
> -			dsp3_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPCTRL);
> +			HVS_WRITE(SCALER_DISPCTRL,
> +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
>   			break;
>   
>   		case 4:
> -			dsp4_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPEOLN);
> +			HVS_WRITE(SCALER_DISPEOLN,
> +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> +
>   			break;
>   
>   		case 5:
> -			dsp5_mux = vc4_state->assigned_channel;
> +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> +				mux = 3;
> +			else
> +				mux = vc4_state->assigned_channel;
> +
> +			reg = HVS_READ(SCALER_DISPDITHER);
> +			HVS_WRITE(SCALER_DISPDITHER,
> +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
>   			break;
>   
>   		default:
>   			break;
>   		}
>   	}
> -
> -	reg = HVS_READ(SCALER_DISPECTRL);
> -	HVS_WRITE(SCALER_DISPECTRL,
> -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPCTRL);
> -	HVS_WRITE(SCALER_DISPCTRL,
> -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPEOLN);
> -	HVS_WRITE(SCALER_DISPEOLN,
> -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> -
> -	reg = HVS_READ(SCALER_DISPDITHER);
> -	HVS_WRITE(SCALER_DISPDITHER,
> -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
>   }
>   
>   static void
> @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		return -EINVAL;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		struct vc4_crtc_state *old_vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);
>   		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 (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> -			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +		/* Nothing to do here, let's skip it */
> +		if ((old_crtc_state->enable && new_crtc_state->enable) ||
> +		    (!old_crtc_state->enable && !new_crtc_state->enable)) {
> +			new_vc4_crtc_state->needs_muxing = false;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
>   
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> +		/* If we're disabling our CRTC, we put back our channel */
> +		if (old_crtc_state->enable && !new_crtc_state->enable) {
> +			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> +		}
>   
>   		/*
>   		 * The problem we have to solve here is that we have
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5488 bytes --]

Hi!

On Mon, Nov 02, 2020 at 05:47:04PM +0900, Hoegeun Kwon wrote:
> Hi Maxime,
> 
> Thanks for V2 patch.
> 
> 
> On 10/28/20 9:41 PM, Maxime Ripard wrote:
> > 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 setting a flag on the CRTC state when the muxing has been
> > changed, and only change the muxing configuration when that flag is there.
> >
> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
> >   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
> >   2 files changed, 50 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index c6208b040f77..c074c0538e57 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -523,6 +523,7 @@ struct vc4_crtc_state {
> >   	struct drm_mm_node mm;
> >   	bool feed_txp;
> >   	bool txp_armed;
> > +	bool needs_muxing;
> >   	unsigned int assigned_channel;
> >   
> >   	struct {
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 2aa726b7422c..409aeb19d210 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   {
> >   	struct drm_crtc_state *crtc_state;
> >   	struct drm_crtc *crtc;
> > -	unsigned char dsp2_mux = 0;
> > -	unsigned char dsp3_mux = 3;
> > -	unsigned char dsp4_mux = 3;
> > -	unsigned char dsp5_mux = 3;
> > +	unsigned char mux;
> >   	unsigned int i;
> >   	u32 reg;
> >   
> > @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
> >   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> >   
> > -		if (!crtc_state->active)
> > +		if (!vc4_state->needs_muxing)
> >   			continue;
> >   
> >   		switch (vc4_crtc->data->hvs_output) {
> >   		case 2:
> > -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			reg = HVS_READ(SCALER_DISPECTRL);
> > +			HVS_WRITE(SCALER_DISPECTRL,
> > +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
> >   			break;
> >   
> >   		case 3:
> > -			dsp3_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPCTRL);
> > +			HVS_WRITE(SCALER_DISPCTRL,
> > +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
> >   			break;
> >   
> >   		case 4:
> > -			dsp4_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPEOLN);
> > +			HVS_WRITE(SCALER_DISPEOLN,
> > +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> > +
> >   			break;
> >   
> >   		case 5:
> > -			dsp5_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPDITHER);
> > +			HVS_WRITE(SCALER_DISPDITHER,
> > +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
> >   			break;
> >   
> >   		default:
> >   			break;
> >   		}
> >   	}
> > -
> > -	reg = HVS_READ(SCALER_DISPECTRL);
> > -	HVS_WRITE(SCALER_DISPECTRL,
> > -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPCTRL);
> > -	HVS_WRITE(SCALER_DISPCTRL,
> > -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPEOLN);
> > -	HVS_WRITE(SCALER_DISPEOLN,
> > -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPDITHER);
> > -	HVS_WRITE(SCALER_DISPDITHER,
> > -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
> >   }
> >   
> >   static void
> > @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
> >   		return -EINVAL;
> >   
> >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +		struct vc4_crtc_state *old_vc4_crtc_state =
> > +			to_vc4_crtc_state(old_crtc_state);
> 
> In my opinion, the old_vc4_crtc_state definition is better to move to 
> patch6.
> Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.

You're totally right, I've fixed it in my third version, thanks!
Maxime

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

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Maxime Ripard]

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

Hi!

On Mon, Nov 02, 2020 at 05:47:04PM +0900, Hoegeun Kwon wrote:
> Hi Maxime,
> 
> Thanks for V2 patch.
> 
> 
> On 10/28/20 9:41 PM, Maxime Ripard wrote:
> > 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 setting a flag on the CRTC state when the muxing has been
> > changed, and only change the muxing configuration when that flag is there.
> >
> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
> >   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
> >   2 files changed, 50 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index c6208b040f77..c074c0538e57 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -523,6 +523,7 @@ struct vc4_crtc_state {
> >   	struct drm_mm_node mm;
> >   	bool feed_txp;
> >   	bool txp_armed;
> > +	bool needs_muxing;
> >   	unsigned int assigned_channel;
> >   
> >   	struct {
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 2aa726b7422c..409aeb19d210 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   {
> >   	struct drm_crtc_state *crtc_state;
> >   	struct drm_crtc *crtc;
> > -	unsigned char dsp2_mux = 0;
> > -	unsigned char dsp3_mux = 3;
> > -	unsigned char dsp4_mux = 3;
> > -	unsigned char dsp5_mux = 3;
> > +	unsigned char mux;
> >   	unsigned int i;
> >   	u32 reg;
> >   
> > @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
> >   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> >   
> > -		if (!crtc_state->active)
> > +		if (!vc4_state->needs_muxing)
> >   			continue;
> >   
> >   		switch (vc4_crtc->data->hvs_output) {
> >   		case 2:
> > -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			reg = HVS_READ(SCALER_DISPECTRL);
> > +			HVS_WRITE(SCALER_DISPECTRL,
> > +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
> >   			break;
> >   
> >   		case 3:
> > -			dsp3_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPCTRL);
> > +			HVS_WRITE(SCALER_DISPCTRL,
> > +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
> >   			break;
> >   
> >   		case 4:
> > -			dsp4_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPEOLN);
> > +			HVS_WRITE(SCALER_DISPEOLN,
> > +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> > +
> >   			break;
> >   
> >   		case 5:
> > -			dsp5_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPDITHER);
> > +			HVS_WRITE(SCALER_DISPDITHER,
> > +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
> >   			break;
> >   
> >   		default:
> >   			break;
> >   		}
> >   	}
> > -
> > -	reg = HVS_READ(SCALER_DISPECTRL);
> > -	HVS_WRITE(SCALER_DISPECTRL,
> > -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPCTRL);
> > -	HVS_WRITE(SCALER_DISPCTRL,
> > -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPEOLN);
> > -	HVS_WRITE(SCALER_DISPEOLN,
> > -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPDITHER);
> > -	HVS_WRITE(SCALER_DISPDITHER,
> > -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
> >   }
> >   
> >   static void
> > @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
> >   		return -EINVAL;
> >   
> >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +		struct vc4_crtc_state *old_vc4_crtc_state =
> > +			to_vc4_crtc_state(old_crtc_state);
> 
> In my opinion, the old_vc4_crtc_state definition is better to move to 
> patch6.
> Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.

You're totally right, I've fixed it in my third version, thanks!
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

Re: [PATCH v2 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Maxime Ripard]

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

Hi!

On Mon, Nov 02, 2020 at 05:47:04PM +0900, Hoegeun Kwon wrote:
> Hi Maxime,
> 
> Thanks for V2 patch.
> 
> 
> On 10/28/20 9:41 PM, Maxime Ripard wrote:
> > 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 setting a flag on the CRTC state when the muxing has been
> > changed, and only change the muxing configuration when that flag is there.
> >
> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |  1 +-
> >   drivers/gpu/drm/vc4/vc4_kms.c | 84 +++++++++++++++++++++---------------
> >   2 files changed, 50 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index c6208b040f77..c074c0538e57 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -523,6 +523,7 @@ struct vc4_crtc_state {
> >   	struct drm_mm_node mm;
> >   	bool feed_txp;
> >   	bool txp_armed;
> > +	bool needs_muxing;
> >   	unsigned int assigned_channel;
> >   
> >   	struct {
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 2aa726b7422c..409aeb19d210 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -224,10 +224,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   {
> >   	struct drm_crtc_state *crtc_state;
> >   	struct drm_crtc *crtc;
> > -	unsigned char dsp2_mux = 0;
> > -	unsigned char dsp3_mux = 3;
> > -	unsigned char dsp4_mux = 3;
> > -	unsigned char dsp5_mux = 3;
> > +	unsigned char mux;
> >   	unsigned int i;
> >   	u32 reg;
> >   
> > @@ -235,50 +232,59 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
> >   		struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state);
> >   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> >   
> > -		if (!crtc_state->active)
> > +		if (!vc4_state->needs_muxing)
> >   			continue;
> >   
> >   		switch (vc4_crtc->data->hvs_output) {
> >   		case 2:
> > -			dsp2_mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			mux = (vc4_state->assigned_channel == 2) ? 0 : 1;
> > +			reg = HVS_READ(SCALER_DISPECTRL);
> > +			HVS_WRITE(SCALER_DISPECTRL,
> > +				  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPECTRL_DSP2_MUX));
> >   			break;
> >   
> >   		case 3:
> > -			dsp3_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPCTRL);
> > +			HVS_WRITE(SCALER_DISPCTRL,
> > +				  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPCTRL_DSP3_MUX));
> >   			break;
> >   
> >   		case 4:
> > -			dsp4_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPEOLN);
> > +			HVS_WRITE(SCALER_DISPEOLN,
> > +				  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPEOLN_DSP4_MUX));
> > +
> >   			break;
> >   
> >   		case 5:
> > -			dsp5_mux = vc4_state->assigned_channel;
> > +			if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> > +				mux = 3;
> > +			else
> > +				mux = vc4_state->assigned_channel;
> > +
> > +			reg = HVS_READ(SCALER_DISPDITHER);
> > +			HVS_WRITE(SCALER_DISPDITHER,
> > +				  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > +				  VC4_SET_FIELD(mux, SCALER_DISPDITHER_DSP5_MUX));
> >   			break;
> >   
> >   		default:
> >   			break;
> >   		}
> >   	}
> > -
> > -	reg = HVS_READ(SCALER_DISPECTRL);
> > -	HVS_WRITE(SCALER_DISPECTRL,
> > -		  (reg & ~SCALER_DISPECTRL_DSP2_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp2_mux, SCALER_DISPECTRL_DSP2_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPCTRL);
> > -	HVS_WRITE(SCALER_DISPCTRL,
> > -		  (reg & ~SCALER_DISPCTRL_DSP3_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp3_mux, SCALER_DISPCTRL_DSP3_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPEOLN);
> > -	HVS_WRITE(SCALER_DISPEOLN,
> > -		  (reg & ~SCALER_DISPEOLN_DSP4_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp4_mux, SCALER_DISPEOLN_DSP4_MUX));
> > -
> > -	reg = HVS_READ(SCALER_DISPDITHER);
> > -	HVS_WRITE(SCALER_DISPDITHER,
> > -		  (reg & ~SCALER_DISPDITHER_DSP5_MUX_MASK) |
> > -		  VC4_SET_FIELD(dsp5_mux, SCALER_DISPDITHER_DSP5_MUX));
> >   }
> >   
> >   static void
> > @@ -769,21 +775,29 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
> >   		return -EINVAL;
> >   
> >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +		struct vc4_crtc_state *old_vc4_crtc_state =
> > +			to_vc4_crtc_state(old_crtc_state);
> 
> In my opinion, the old_vc4_crtc_state definition is better to move to 
> patch6.
> Build error occurs in patch6 because old_vc4_crtc_state is used in patch6.

You're totally right, I've fixed it in my third version, thanks!
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