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

[PATCH v3 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
  - Added Hoegeun Kwon's tags
  - Fixed a build bisection error
  - Cleaned up the private state using drmm_add_action_or_reset
  - Rebased on current linux next

Maxime Ripard (7):
  drm/vc4: kms: Switch to drmm_add_action_or_reset
  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: 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 | 247 +++++++++++++++++++++++++---------
 2 files changed, 185 insertions(+), 64 deletions(-)

-- 
2.28.0

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

[PATCH v3 1/7] drm/vc4: kms: Switch to drmm_add_action_or_reset

[Maxime Ripard]

Even though it was pointed in the review by Daniel, and I thought to have
fixed it while applying the patches, but it turns out I forgot to commit
the fixes in the process. Properly fix it this time.

Fixes: dcda7c28bff2 ("drm/vc4: kms: Add functions to create the state objects")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2b951cae04ad..44db31e16e91 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -113,7 +113,7 @@ static int vc4_ctm_obj_init(struct vc4_dev *vc4)
 	drm_atomic_private_obj_init(&vc4->base, &vc4->ctm_manager, &ctm_state->base,
 				    &vc4_ctm_state_funcs);
 
-	return drmm_add_action(&vc4->base, vc4_ctm_obj_fini, NULL);
+	return drmm_add_action_or_reset(&vc4->base, vc4_ctm_obj_fini, NULL);
 }
 
 /* Converts a DRM S31.32 value to the HW S0.9 format. */
@@ -657,7 +657,7 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 				    &load_state->base,
 				    &vc4_load_tracker_state_funcs);
 
-	return drmm_add_action(&vc4->base, vc4_load_tracker_obj_fini, NULL);
+	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
 #define NUM_OUTPUTS  6
-- 
2.28.0

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

Re: [PATCH v3 1/7] drm/vc4: kms: Switch to drmm_add_action_or_reset

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1737 bytes --]



Am 05.11.20 um 14:56 schrieb Maxime Ripard:
> Even though it was pointed in the review by Daniel, and I thought to have
> fixed it while applying the patches, but it turns out I forgot to commit
> the fixes in the process. Properly fix it this time.
> 
> Fixes: dcda7c28bff2 ("drm/vc4: kms: Add functions to create the state objects")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2b951cae04ad..44db31e16e91 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -113,7 +113,7 @@ static int vc4_ctm_obj_init(struct vc4_dev *vc4)
>   	drm_atomic_private_obj_init(&vc4->base, &vc4->ctm_manager, &ctm_state->base,
>   				    &vc4_ctm_state_funcs);
>   
> -	return drmm_add_action(&vc4->base, vc4_ctm_obj_fini, NULL);
> +	return drmm_add_action_or_reset(&vc4->base, vc4_ctm_obj_fini, NULL);
>   }
>   
>   /* Converts a DRM S31.32 value to the HW S0.9 format. */
> @@ -657,7 +657,7 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   				    &load_state->base,
>   				    &vc4_load_tracker_state_funcs);
>   
> -	return drmm_add_action(&vc4->base, vc4_load_tracker_obj_fini, NULL);
> +	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
>   #define NUM_OUTPUTS  6
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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

[PATCH v3 2/7] drm/vc4: kms: Remove useless define

[Maxime Ripard]

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

Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
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 44db31e16e91..4b558ccb18fe 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -660,7 +660,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
-#define NUM_OUTPUTS  6
 #define NUM_CHANNELS 3
 
 static int
-- 
2.28.0

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

Re: [PATCH v3 2/7] drm/vc4: kms: Remove useless define

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1068 bytes --]



Am 05.11.20 um 14:56 schrieb Maxime Ripard:
> NUM_OUTPUTS isn't used anymore, let's remove it.
> 
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   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 44db31e16e91..4b558ccb18fe 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -660,7 +660,6 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
> -#define NUM_OUTPUTS  6
>   #define NUM_CHANNELS 3
>   
>   static int
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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

[PATCH v3 3/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.

Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4b558ccb18fe..ad69c70f66a2 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;
@@ -660,12 +662,10 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
-#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;
-- 
2.28.0

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

Re: [PATCH v3 3/7] drm/vc4: kms: Rename NUM_CHANNELS

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1780 bytes --]



Am 05.11.20 um 14:56 schrieb 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.
> 
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4b558ccb18fe..ad69c70f66a2 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;
> @@ -660,12 +662,10 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
> -#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;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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

[PATCH v3 4/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.

Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
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 ad69c70f66a2..bb2efc5d2d01 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -662,13 +662,13 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
-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
@@ -741,6 +741,18 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 
+	return 0;
+}
+
+static int
+vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = vc4_pv_muxing_atomic_check(dev, state);
+	if (ret)
+		return ret;
+
 	ret = vc4_ctm_atomic_check(dev, state);
 	if (ret < 0)
 		return ret;
-- 
2.28.0

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

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

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2266 bytes --]



Am 05.11.20 um 14:56 schrieb 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.
> 
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> 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 ad69c70f66a2..bb2efc5d2d01 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -662,13 +662,13 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
> -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;

Thanks for fixing the type. It's always itching me when people use 
signed types for counters and array indices. It's just... not right. :) 
If you want to be super-correct you might as well make it size_t. Anyway

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

>   
>   	/*
>   	 * Since the HVS FIFOs are shared across all the pixelvalves and
> @@ -741,6 +741,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;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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

[PATCH v3 5/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.

Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
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 bb2efc5d2d01..499c6914fce4 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -662,6 +662,28 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
+/*
+ * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
+ * the TXP (and therefore all the CRTCs found on that platform).
+ *
+ * The naive (and our initial) implementation would just iterate over
+ * all the active CRTCs, try to find a suitable FIFO, and then remove it
+ * from the available FIFOs pool. However, there's a few corner cases
+ * that need to be considered:
+ *
+ * - When running in a dual-display setup (so with two CRTCs involved),
+ *   we can update the state of a single CRTC (for example by changing
+ *   its mode using xrandr under X11) without affecting the other. In
+ *   this case, the other CRTC wouldn't be in the state at all, so we
+ *   need to consider all the running CRTCs in the DRM device to assign
+ *   a FIFO, not just the one in the state.
+ *
+ * - Since we need the pixelvalve to be disabled and enabled back when
+ *   the FIFO is changed, we should keep the FIFO assigned for as long
+ *   as the CRTC is enabled, only considering it free again once that
+ *   CRTC has been disabled. This can be tested by booting X11 on a
+ *   single display, and changing the resolution down and then back up.
+ */
 static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 				      struct drm_atomic_state *state)
 {
-- 
2.28.0

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

Re: [PATCH v3 5/7] drm/vc4: kms: Document the muxing corner cases

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2767 bytes --]

Hi,

A few suggestions below. But I'm not a native speaker.

Am 05.11.20 um 14:56 schrieb 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.
> 
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> 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 bb2efc5d2d01..499c6914fce4 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -662,6 +662,28 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
> +/*
> + * The BCM2711 HVS has up to 7 output connected to the pixelvalves and

'7 outputs'

Is it 'pixelvalves' or 'pixel valves'?

> + * 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

I'd write. 'and remove it from the pool of available FIFOs'. Sounds more 
natural to me.

'there are a few'

> + * 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.
> + */

With my suggestions considered,

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

>   static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   				      struct drm_atomic_state *state)
>   {
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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 v3 5/7] drm/vc4: kms: Document the muxing corner cases

[Maxime Ripard]

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

Hi Thomas,

Thanks again for your review

On Thu, Nov 19, 2020 at 09:11:58AM +0100, Thomas Zimmermann wrote:
> Hi,
> 
> A few suggestions below. But I'm not a native speaker.
> 
> Am 05.11.20 um 14:56 schrieb 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.
> > 
> > Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > 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 bb2efc5d2d01..499c6914fce4 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -662,6 +662,28 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> >   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
> >   }
> > +/*
> > + * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
> 
> '7 outputs'
> 
> Is it 'pixelvalves' or 'pixel valves'?

The documentation uses both, but the driver uses the former all the
time.

> 
> > + * 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
> 
> I'd write. 'and remove it from the pool of available FIFOs'. Sounds more
> natural to me.
> 
> 'there are a few'
> 
> > + * 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.
> > + */
> 
> With my suggestions considered,
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

I've applied it with your comments fixed. 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

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

Since vc4 has a semaphore (with a value of 1, so a lock) in its commit
implementation to serialize all the commits, even the nonblocking ones, we
are free from the use-after-free race if two subsequent commits are not ran
in their submission order.

Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |   1 +
 drivers/gpu/drm/vc4/vc4_kms.c | 124 +++++++++++++++++++++++++++-------
 2 files changed, 100 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bdbb9540d47d..014113823647 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -219,6 +219,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 499c6914fce4..0a231ae500e5 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;
@@ -662,6 +673,70 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
 	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
 }
 
+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 void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+
+	drm_atomic_private_obj_fini(&vc4->hvs_channels);
+}
+
+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->base, &vc4->hvs_channels,
+				    &state->base,
+				    &vc4_hvs_state_funcs);
+
+	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
+}
+
+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).
@@ -678,6 +753,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
@@ -687,46 +770,33 @@ 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 *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)
+		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
@@ -752,12 +822,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;
 		}
@@ -841,6 +911,10 @@ int vc4_kms_load(struct drm_device *dev)
 	if (ret)
 		return ret;
 
+	ret = vc4_hvs_channels_obj_init(vc4);
+	if (ret)
+		return ret;
+
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
-- 
2.28.0

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

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

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 9701 bytes --]

Hi

Am 05.11.20 um 14:56 schrieb 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.
> 
> Since vc4 has a semaphore (with a value of 1, so a lock) in its commit
> implementation to serialize all the commits, even the nonblocking ones, we
> are free from the use-after-free race if two subsequent commits are not ran
> in their submission order.
> 
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.h |   1 +
>   drivers/gpu/drm/vc4/vc4_kms.c | 124 +++++++++++++++++++++++++++-------
>   2 files changed, 100 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index bdbb9540d47d..014113823647 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -219,6 +219,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 499c6914fce4..0a231ae500e5 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;
> @@ -662,6 +673,70 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
>   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
>   }
>   
> +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 void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> +	drm_atomic_private_obj_fini(&vc4->hvs_channels);
> +}
> +
> +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->base, &vc4->hvs_channels,
> +				    &state->base,
> +				    &vc4_hvs_state_funcs);
> +
> +	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
> +}
> +
> +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).
> @@ -678,6 +753,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
> @@ -687,46 +770,33 @@ 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;

I found this confusing. It's technically correct, but from hvs_state is 
not clear that it's the new state. Maybe call it hvs_new_state.

If you want to be pedantic, maybe split the creation of the new state 
from the usage. Call vc4_hvs_get_global_state() at the top of 
vc4_atomic_check() to make the new state. (Maybe with a short comment.) 
And here only call an equivalent of 
drm_atomic_get_new_private_obj_state() for hvs_channels.

In any case

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

>   
>   	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)
> +		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
> @@ -752,12 +822,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;
>   		}
> @@ -841,6 +911,10 @@ int vc4_kms_load(struct drm_device *dev)
>   	if (ret)
>   		return ret;
>   
> +	ret = vc4_hvs_channels_obj_init(vc4);
> +	if (ret)
> +		return ret;
> +
>   	drm_mode_config_reset(dev);
>   
>   	drm_kms_helper_poll_init(dev);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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 v3 6/7] drm/vc4: kms: Store the unassigned channel list in the state

[Maxime Ripard]

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

Hi Thomas,

On Thu, Nov 19, 2020 at 09:59:15AM +0100, Thomas Zimmermann wrote:
> Am 05.11.20 um 14:56 schrieb 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.
> > 
> > Since vc4 has a semaphore (with a value of 1, so a lock) in its commit
> > implementation to serialize all the commits, even the nonblocking ones, we
> > are free from the use-after-free race if two subsequent commits are not ran
> > in their submission order.
> > 
> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> > Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |   1 +
> >   drivers/gpu/drm/vc4/vc4_kms.c | 124 +++++++++++++++++++++++++++-------
> >   2 files changed, 100 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index bdbb9540d47d..014113823647 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -219,6 +219,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 499c6914fce4..0a231ae500e5 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;
> > @@ -662,6 +673,70 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> >   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
> >   }
> > +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 void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +
> > +	drm_atomic_private_obj_fini(&vc4->hvs_channels);
> > +}
> > +
> > +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->base, &vc4->hvs_channels,
> > +				    &state->base,
> > +				    &vc4_hvs_state_funcs);
> > +
> > +	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
> > +}
> > +
> > +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).
> > @@ -678,6 +753,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
> > @@ -687,46 +770,33 @@ 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;
> 
> I found this confusing. It's technically correct, but from hvs_state is not
> clear that it's the new state. Maybe call it hvs_new_state.
> 
> If you want to be pedantic, maybe split the creation of the new state from
> the usage. Call vc4_hvs_get_global_state() at the top of vc4_atomic_check()
> to make the new state. (Maybe with a short comment.) And here only call an
> equivalent of drm_atomic_get_new_private_obj_state() for hvs_channels.
> 
> In any case
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

That works for me, I'll change it.

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

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

[Maxime Ripard]

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

Hi!

On Thu, Nov 19, 2020 at 09:59:15AM +0100, Thomas Zimmermann wrote:
> Am 05.11.20 um 14:56 schrieb 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.
> > 
> > Since vc4 has a semaphore (with a value of 1, so a lock) in its commit
> > implementation to serialize all the commits, even the nonblocking ones, we
> > are free from the use-after-free race if two subsequent commits are not ran
> > in their submission order.
> > 
> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> > Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |   1 +
> >   drivers/gpu/drm/vc4/vc4_kms.c | 124 +++++++++++++++++++++++++++-------
> >   2 files changed, 100 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index bdbb9540d47d..014113823647 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -219,6 +219,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 499c6914fce4..0a231ae500e5 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;
> > @@ -662,6 +673,70 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> >   	return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
> >   }
> > +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 void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +
> > +	drm_atomic_private_obj_fini(&vc4->hvs_channels);
> > +}
> > +
> > +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->base, &vc4->hvs_channels,
> > +				    &state->base,
> > +				    &vc4_hvs_state_funcs);
> > +
> > +	return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
> > +}
> > +
> > +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).
> > @@ -678,6 +753,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
> > @@ -687,46 +770,33 @@ 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;
> 
> I found this confusing. It's technically correct, but from hvs_state is not
> clear that it's the new state. Maybe call it hvs_new_state.

Yeah, you're right, I'll change it

> If you want to be pedantic, maybe split the creation of the new state from
> the usage. Call vc4_hvs_get_global_state() at the top of vc4_atomic_check()
> to make the new state. (Maybe with a short comment.) And here only call an
> equivalent of drm_atomic_get_new_private_obj_state() for hvs_channels.

There's other private states in that driver, and the other states are
using the same construct here, I did so to remain consistent

> In any case
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

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

[PATCH v3 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")
Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.h |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c | 82 ++++++++++++++++++++---------------
 2 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 014113823647..325b53ff11b3 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -524,6 +524,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 0a231ae500e5..7ef164afa9e2 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -226,10 +226,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;
 
@@ -237,50 +234,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
@@ -787,17 +793,23 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
 
+		/* 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;
+		}
+
+		/* Muxing will need to be modified, mark it as such */
+		new_vc4_crtc_state->needs_muxing = true;
+
+		/* 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;
 		}
 
-		if (!new_crtc_state->enable)
-			continue;
-
-		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
-			continue;
-
 		/*
 		 * The problem we have to solve here is that we have
 		 * up to 7 encoders, connected to up to 6 CRTCs.
-- 
2.28.0

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

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

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 6615 bytes --]

Hi

Am 05.11.20 um 14:56 schrieb 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")
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.h |  1 +
>   drivers/gpu/drm/vc4/vc4_kms.c | 82 ++++++++++++++++++++---------------
>   2 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 014113823647..325b53ff11b3 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -524,6 +524,7 @@ struct vc4_crtc_state {
>   	struct drm_mm_node mm;
>   	bool feed_txp;
>   	bool txp_armed;
> +	bool needs_muxing;

Maybe rather 'update_muxing'.

More generally, I'd separate fields that contain actual CRTC state, such 
assigned_channel, from those that only contain transitional state during 
update-commit, such as 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 0a231ae500e5..7ef164afa9e2 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -226,10 +226,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;
>   
> @@ -237,50 +234,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
> @@ -787,17 +793,23 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>   		unsigned int matching_channels;
>   
> +		/* 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)) {

'if (old_crtc_state->enable == new_crtc_state->enable)' ?

> +			new_vc4_crtc_state->needs_muxing = false;

Shouldn't this be cleared after duplicating the crtc state? No need to 
clear it then.

> +			continue;
> +		}
> +
> +		/* Muxing will need to be modified, mark it as such */
> +		new_vc4_crtc_state->needs_muxing = true;
> +
> +		/* If we're disabling our CRTC, we put back our channel */
>   		if (old_crtc_state->enable && !new_crtc_state->enable) {

You've already checked that enable is different, so you can leave out 
the test on old_crtc_state->enable

With my suggestions considered

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>


>   			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
>   			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> +			continue;
>   		}
>   
> -		if (!new_crtc_state->enable)
> -			continue;
> -
> -		if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> -			continue;
> -
>   		/*
>   		 * The problem we have to solve here is that we have
>   		 * up to 7 encoders, connected to up to 6 CRTCs.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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 v3 7/7] drm/vc4: kms: Don't disable the muxing of an active CRTC

[Maxime Ripard]

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

On Thu, Nov 19, 2020 at 10:12:43AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.11.20 um 14:56 schrieb 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")
> > Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.h |  1 +
> >   drivers/gpu/drm/vc4/vc4_kms.c | 82 ++++++++++++++++++++---------------
> >   2 files changed, 48 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index 014113823647..325b53ff11b3 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -524,6 +524,7 @@ struct vc4_crtc_state {
> >   	struct drm_mm_node mm;
> >   	bool feed_txp;
> >   	bool txp_armed;
> > +	bool needs_muxing;
> 
> Maybe rather 'update_muxing'.
> 
> More generally, I'd separate fields that contain actual CRTC state, such
> assigned_channel, from those that only contain transitional state during
> update-commit, such as needs_muxing.

How would you separate them?

I'll use your other suggestions, 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

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

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2206 bytes --]

Hi

Am 19.11.20 um 15:32 schrieb Maxime Ripard:
> On Thu, Nov 19, 2020 at 10:12:43AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 05.11.20 um 14:56 schrieb 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")
>>> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>> ---
>>>    drivers/gpu/drm/vc4/vc4_drv.h |  1 +
>>>    drivers/gpu/drm/vc4/vc4_kms.c | 82 ++++++++++++++++++++---------------
>>>    2 files changed, 48 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>>> index 014113823647..325b53ff11b3 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>>> @@ -524,6 +524,7 @@ struct vc4_crtc_state {
>>>    	struct drm_mm_node mm;
>>>    	bool feed_txp;
>>>    	bool txp_armed;
>>> +	bool needs_muxing;
>>
>> Maybe rather 'update_muxing'.
>>
>> More generally, I'd separate fields that contain actual CRTC state, such
>> assigned_channel, from those that only contain transitional state during
>> update-commit, such as needs_muxing.
> 
> How would you separate them?
> 
> I'll use your other suggestions, thanks!

I'd put the transitional state at the end of the struct and put a 
comment above. Something like 'Transitional state below; only valid 
during atomic commits'.

Best regards
Thomas

> Maxime
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 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