[PATCH v2 0/7] vc4: Convert to drm_atomic_helper_commit

[PATCH v2 0/7] vc4: Convert to drm_atomic_helper_commit

[Maxime Ripard]

Hi,

Here's a conversion of vc4 to remove the hand-rolled atomic_commit helper from
vc4 in favour of the generic one.

This requires some rework of vc4, but also a new hook and some documentation
for corner-cases in the DRM core that have been reported and explained by
Daniel recently.

Let me know what you think,
Maxime

Changes from v1:
  - Addressed the comments from Dave and Thomas on the documentation
  - s/last_user/pending_commit/
  - Check that the commit is not NULL before waiting on it
  - Fixed a compilation error on an intermediate patch
  - Drop the assigned_channels variable redundant with the in_use variable

Maxime Ripard (7):
  drm: Introduce an atomic_commit_setup function
  drm: Document use-after-free gotcha with private objects
  drm/vc4: Simplify a bit the global atomic_check
  drm/vc4: kms: Wait on previous FIFO users before a commit
  drm/vc4: kms: Remove unassigned_channels from the HVS state
  drm/vc4: kms: Remove async modeset semaphore
  drm/vc4: kms: Convert to atomic helpers

 drivers/gpu/drm/drm_atomic_helper.c      |   9 +
 drivers/gpu/drm/vc4/vc4_crtc.c           |  13 --
 drivers/gpu/drm/vc4/vc4_drv.h            |   2 -
 drivers/gpu/drm/vc4/vc4_kms.c            | 248 +++++++++++------------
 include/drm/drm_atomic.h                 |  20 ++
 include/drm/drm_modeset_helper_vtables.h |  21 ++
 6 files changed, 172 insertions(+), 141 deletions(-)

-- 
2.28.0

[PATCH v2 1/7] drm: Introduce an atomic_commit_setup function

[Maxime Ripard]

Private objects storing a state shared across all CRTCs need to be
carefully handled to avoid a use-after-free issue.

The proper way to do this to track all the commits using that shared
state and wait for the previous commits to be done before going on with
the current one to avoid the reordering of commits that could occur.

However, this commit setup needs to be done after
drm_atomic_helper_setup_commit(), because before the CRTC commit
structure hasn't been allocated before, and before the workqueue is
scheduled, because we would be potentially reordered already otherwise.

That means that drivers currently have to roll their own
drm_atomic_helper_commit() function, even though it would be identical
if not for the commit setup.

Let's introduce a hook to do so that would be called as part of
drm_atomic_helper_commit, allowing us to reuse the atomic helpers.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic_helper.c      |  9 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f9170b4b22e7..f754e21b96eb 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2034,6 +2034,9 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
  * should always call this function from their
  * &drm_mode_config_funcs.atomic_commit hook.
  *
+ * Drivers that need to extend the commit setup to private objects can use the
+ * &drm_mode_config_helper_funcs.atomic_commit_setup hook.
+ *
  * To be able to use this support drivers need to use a few more helper
  * functions. drm_atomic_helper_wait_for_dependencies() must be called before
  * actually committing the hardware state, and for nonblocking commits this call
@@ -2077,8 +2080,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
+	const struct drm_mode_config_helper_funcs *funcs;
 	int i, ret;
 
+	funcs = state->dev->mode_config.helper_private;
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
 		if (!commit)
@@ -2155,6 +2161,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		new_plane_state->commit = drm_crtc_commit_get(commit);
 	}
 
+	if (funcs && funcs->atomic_commit_setup)
+		return funcs->atomic_commit_setup(state);
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 4efec30f8bad..0ebb3f191bbc 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1406,6 +1406,27 @@ struct drm_mode_config_helper_funcs {
 	 * drm_atomic_helper_commit_tail().
 	 */
 	void (*atomic_commit_tail)(struct drm_atomic_state *state);
+
+	/**
+	 * @atomic_commit_setup:
+	 *
+	 * This hook is used by the default atomic_commit() hook implemented in
+	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
+	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
+	 * is not used by the atomic helpers.
+	 *
+	 * This function is called at the end of
+	 * drm_atomic_helper_setup_commit(), so once the commit has been
+	 * properly setup across the generic DRM object states. It allows
+	 * drivers to do some additional commit tracking that isn't related to a
+	 * CRTC, plane or connector, tracked in a &drm_private_obj structure.
+	 *
+	 * Note that the documentation of &drm_private_obj has more details on
+	 * how one should implement this.
+	 *
+	 * This hook is optional.
+	 */
+	int (*atomic_commit_setup)(struct drm_atomic_state *state);
 };
 
 #endif
-- 
2.28.0

Re: [PATCH v2 1/7] drm: Introduce an atomic_commit_setup function

[Daniel Vetter]

On Fri, Dec 4, 2020 at 4:11 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Private objects storing a state shared across all CRTCs need to be
> carefully handled to avoid a use-after-free issue.
>
> The proper way to do this to track all the commits using that shared
> state and wait for the previous commits to be done before going on with
> the current one to avoid the reordering of commits that could occur.
>
> However, this commit setup needs to be done after
> drm_atomic_helper_setup_commit(), because before the CRTC commit
> structure hasn't been allocated before, and before the workqueue is
> scheduled, because we would be potentially reordered already otherwise.
>
> That means that drivers currently have to roll their own
> drm_atomic_helper_commit() function, even though it would be identical
> if not for the commit setup.
>
> Let's introduce a hook to do so that would be called as part of
> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

r-b: me too

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  9 +++++++++
>  include/drm/drm_modeset_helper_vtables.h | 21 +++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f9170b4b22e7..f754e21b96eb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2034,6 +2034,9 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
>   * should always call this function from their
>   * &drm_mode_config_funcs.atomic_commit hook.
>   *
> + * Drivers that need to extend the commit setup to private objects can use the
> + * &drm_mode_config_helper_funcs.atomic_commit_setup hook.
> + *
>   * To be able to use this support drivers need to use a few more helper
>   * functions. drm_atomic_helper_wait_for_dependencies() must be called before
>   * actually committing the hardware state, and for nonblocking commits this call
> @@ -2077,8 +2080,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>         struct drm_plane *plane;
>         struct drm_plane_state *old_plane_state, *new_plane_state;
>         struct drm_crtc_commit *commit;
> +       const struct drm_mode_config_helper_funcs *funcs;
>         int i, ret;
>
> +       funcs = state->dev->mode_config.helper_private;
> +
>         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>                 commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>                 if (!commit)
> @@ -2155,6 +2161,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>                 new_plane_state->commit = drm_crtc_commit_get(commit);
>         }
>
> +       if (funcs && funcs->atomic_commit_setup)
> +               return funcs->atomic_commit_setup(state);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 4efec30f8bad..0ebb3f191bbc 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1406,6 +1406,27 @@ struct drm_mode_config_helper_funcs {
>          * drm_atomic_helper_commit_tail().
>          */
>         void (*atomic_commit_tail)(struct drm_atomic_state *state);
> +
> +       /**
> +        * @atomic_commit_setup:
> +        *
> +        * This hook is used by the default atomic_commit() hook implemented in
> +        * drm_atomic_helper_commit() together with the nonblocking helpers (see
> +        * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
> +        * is not used by the atomic helpers.
> +        *
> +        * This function is called at the end of
> +        * drm_atomic_helper_setup_commit(), so once the commit has been
> +        * properly setup across the generic DRM object states. It allows
> +        * drivers to do some additional commit tracking that isn't related to a
> +        * CRTC, plane or connector, tracked in a &drm_private_obj structure.
> +        *
> +        * Note that the documentation of &drm_private_obj has more details on
> +        * how one should implement this.
> +        *
> +        * This hook is optional.
> +        */
> +       int (*atomic_commit_setup)(struct drm_atomic_state *state);
>  };
>
>  #endif
> --
> 2.28.0
>


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

[PATCH v2 2/7] drm: Document use-after-free gotcha with private objects

[Maxime Ripard]

The private objects have a gotcha that could result in a use-after-free,
make sure it's properly documented.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 include/drm/drm_atomic.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d07c851d255b..5d34c1df03f3 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -248,6 +248,26 @@ struct drm_private_state_funcs {
  *    drm_dev_register()
  * 2/ all calls to drm_atomic_private_obj_fini() must be done after calling
  *    drm_dev_unregister()
+ *
+ * If that private object is used to store a state shared by multiple
+ * CRTCs, proper care must be taken to ensure that non-blocking commits are
+ * properly ordered to avoid a use-after-free issue.
+ *
+ * Indeed, assuming a sequence of two non-blocking &drm_atomic_commit on two
+ * different &drm_crtc using different &drm_plane and &drm_connector, so with no
+ * resources shared, there's no guarantee on which commit is going to happen
+ * first. However, the second &drm_atomic_commit will consider the first
+ * &drm_private_obj its old state, and will be in charge of freeing it whenever
+ * the second &drm_atomic_commit is done.
+ *
+ * If the first &drm_atomic_commit happens after it, it will consider its
+ * &drm_private_obj the new state and will be likely to access it, resulting in
+ * an access to a freed memory region. Drivers should store (and get a reference
+ * to) the &drm_crtc_commit structure in our private state in
+ * &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that
+ * commit to complete as the first step of
+ * &drm_mode_config_helper_funcs.atomic_commit_tail, similar to
+ * drm_atomic_helper_wait_for_dependencies().
  */
 struct drm_private_obj {
 	/**
-- 
2.28.0

[PATCH v2 3/7] drm/vc4: Simplify a bit the global atomic_check

[Maxime Ripard]

When we can't allocate a new channel, we can simply return instead of
having to handle both cases, and that simplifies a bit the code.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ba310c0ab5f6..8937eb0b751d 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -794,6 +794,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 			to_vc4_crtc_state(new_crtc_state);
 		struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 		unsigned int matching_channels;
+		unsigned int channel;
 
 		/* Nothing to do here, let's skip it */
 		if (old_crtc_state->enable == new_crtc_state->enable)
@@ -834,14 +835,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		 * but it works so far.
 		 */
 		matching_channels = hvs_new_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;
-			hvs_new_state->unassigned_channels &= ~BIT(channel);
-		} else {
+		if (!matching_channels)
 			return -EINVAL;
-		}
+
+		channel = ffs(matching_channels) - 1;
+		new_vc4_crtc_state->assigned_channel = channel;
+		hvs_new_state->unassigned_channels &= ~BIT(channel);
 	}
 
 	return 0;
-- 
2.28.0

[PATCH v2 4/7] drm/vc4: kms: Wait on previous FIFO users before a commit

[Maxime Ripard]

If we're having two subsequent, non-blocking, commits on two different
CRTCs that share no resources, there's no guarantee on the order of
execution of both commits.

However, the second one will consider the first one as the old state,
and will be in charge of freeing it once that second commit is done.

If the first commit happens after that second commit, it might access
some resources related to its state that has been freed, resulting in a
use-after-free bug.

The standard DRM objects are protected against this, but our HVS private
state isn't so let's make sure we wait for all the previous FIFO users
to finish their commit before going with our own.

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

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 8937eb0b751d..fdd698df5fbe 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 struct vc4_hvs_state {
 	struct drm_private_state base;
 	unsigned int unassigned_channels;
+
+	struct {
+		unsigned in_use: 1;
+		struct drm_crtc_commit *pending_commit;
+	} fifo_state[HVS_NUM_CHANNELS];
 };
 
 static struct vc4_hvs_state *
@@ -182,6 +187,32 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
 		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
 }
 
+static struct vc4_hvs_state *
+vc4_hvs_get_new_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_new_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
+static struct vc4_hvs_state *
+vc4_hvs_get_old_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_old_private_obj_state(state, &vc4->hvs_channels);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_vc4_hvs_state(priv_state);
+}
+
 static struct vc4_hvs_state *
 vc4_hvs_get_global_state(struct drm_atomic_state *state)
 {
@@ -308,8 +339,10 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct vc4_hvs *hvs = vc4->hvs;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
+	struct vc4_hvs_state *old_hvs_state;
 	int i;
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -329,6 +362,36 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_wait_for_dependencies(state);
 
+	old_hvs_state = vc4_hvs_get_old_global_state(state);
+	if (!old_hvs_state)
+		return;
+
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+		struct vc4_crtc_state *vc4_crtc_state =
+			to_vc4_crtc_state(old_crtc_state);
+		struct drm_crtc_commit *commit;
+		unsigned int channel = vc4_crtc_state->assigned_channel;
+		unsigned long done;
+
+		if (channel == VC4_HVS_CHANNEL_DISABLED)
+			continue;
+
+		if (!old_hvs_state->fifo_state[channel].in_use)
+			continue;
+
+		commit = old_hvs_state->fifo_state[i].pending_commit;
+		if (!commit)
+			continue;
+
+		done = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
+		if (!done)
+			drm_err(dev, "Timed out waiting for hw_done\n");
+
+		done = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
+		if (!done)
+			drm_err(dev, "Timed out waiting for flip_done\n");
+	}
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	vc4_ctm_commit(vc4, state);
@@ -368,6 +431,36 @@ static void commit_work(struct work_struct *work)
 	vc4_atomic_complete_commit(state);
 }
 
+static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct vc4_hvs_state *hvs_state;
+	struct drm_crtc *crtc;
+	unsigned int i;
+
+	hvs_state = vc4_hvs_get_new_global_state(state);
+	if (!hvs_state)
+		return -EINVAL;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct vc4_crtc_state *vc4_crtc_state =
+			to_vc4_crtc_state(crtc_state);
+		unsigned int channel =
+			vc4_crtc_state->assigned_channel;
+
+		if (channel == VC4_HVS_CHANNEL_DISABLED)
+			continue;
+
+		if (!hvs_state->fifo_state[channel].in_use)
+			continue;
+
+		hvs_state->fifo_state[channel].pending_commit =
+			drm_crtc_commit_get(crtc_state->commit);
+	}
+
+	return 0;
+}
+
 /**
  * vc4_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -697,6 +790,7 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 {
 	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
 	struct vc4_hvs_state *state;
+	unsigned int i;
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
@@ -706,6 +800,16 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 
 	state->unassigned_channels = old_state->unassigned_channels;
 
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
+
+		if (!old_state->fifo_state[i].pending_commit)
+			continue;
+
+		state->fifo_state[i].pending_commit =
+			drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
+	}
+
 	return &state->base;
 }
 
@@ -713,6 +817,14 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
 					   struct drm_private_state *state)
 {
 	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
+	unsigned int i;
+
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		if (!hvs_state->fifo_state[i].pending_commit)
+			continue;
+
+		drm_crtc_commit_put(hvs_state->fifo_state[i].pending_commit);
+	}
 
 	kfree(hvs_state);
 }
@@ -805,7 +917,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 
 		/* If we're disabling our CRTC, we put back our channel */
 		if (!new_crtc_state->enable) {
-			hvs_new_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
+			channel = old_vc4_crtc_state->assigned_channel;
+
+			hvs_new_state->unassigned_channels |= BIT(channel);
+			hvs_new_state->fifo_state[channel].in_use = false;
 			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
 		}
@@ -841,6 +956,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		channel = ffs(matching_channels) - 1;
 		new_vc4_crtc_state->assigned_channel = channel;
 		hvs_new_state->unassigned_channels &= ~BIT(channel);
+		hvs_new_state->fifo_state[channel].in_use = true;
 	}
 
 	return 0;
@@ -866,6 +982,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	return vc4_load_tracker_atomic_check(state);
 }
 
+static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
+	.atomic_commit_setup	= vc4_atomic_commit_setup,
+};
+
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.atomic_check = vc4_atomic_check,
 	.atomic_commit = vc4_atomic_commit,
@@ -909,6 +1029,7 @@ int vc4_kms_load(struct drm_device *dev)
 	}
 
 	dev->mode_config.funcs = &vc4_mode_funcs;
+	dev->mode_config.helper_private = &vc4_mode_config_helpers;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.async_page_flip = true;
 	dev->mode_config.allow_fb_modifiers = true;
-- 
2.28.0

Re: [PATCH v2 4/7] drm/vc4: kms: Wait on previous FIFO users before a commit

[Daniel Vetter]

On Fri, Dec 04, 2020 at 04:11:35PM +0100, Maxime Ripard wrote:
> If we're having two subsequent, non-blocking, commits on two different
> CRTCs that share no resources, there's no guarantee on the order of
> execution of both commits.
> 
> However, the second one will consider the first one as the old state,
> and will be in charge of freeing it once that second commit is done.
> 
> If the first commit happens after that second commit, it might access
> some resources related to its state that has been freed, resulting in a
> use-after-free bug.
> 
> The standard DRM objects are protected against this, but our HVS private
> state isn't so let's make sure we wait for all the previous FIFO users
> to finish their commit before going with our own.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_kms.c | 123 +++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 8937eb0b751d..fdd698df5fbe 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>  struct vc4_hvs_state {
>  	struct drm_private_state base;
>  	unsigned int unassigned_channels;
> +
> +	struct {
> +		unsigned in_use: 1;
> +		struct drm_crtc_commit *pending_commit;
> +	} fifo_state[HVS_NUM_CHANNELS];
>  };
>  
>  static struct vc4_hvs_state *
> @@ -182,6 +187,32 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
>  		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
>  }
>  
> +static struct vc4_hvs_state *
> +vc4_hvs_get_new_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_new_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
> +static struct vc4_hvs_state *
> +vc4_hvs_get_old_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_old_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
>  static struct vc4_hvs_state *
>  vc4_hvs_get_global_state(struct drm_atomic_state *state)
>  {
> @@ -308,8 +339,10 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	struct vc4_hvs *hvs = vc4->hvs;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
> +	struct vc4_hvs_state *old_hvs_state;
>  	int i;
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -329,6 +362,36 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_wait_for_dependencies(state);
>  
> +	old_hvs_state = vc4_hvs_get_old_global_state(state);
> +	if (!old_hvs_state)
> +		return;
> +
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);
> +		struct drm_crtc_commit *commit;
> +		unsigned int channel = vc4_crtc_state->assigned_channel;
> +		unsigned long done;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!old_hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		commit = old_hvs_state->fifo_state[i].pending_commit;
> +		if (!commit)
> +			continue;
> +
> +		done = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
> +		if (!done)
> +			drm_err(dev, "Timed out waiting for hw_done\n");
> +
> +		done = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> +		if (!done)
> +			drm_err(dev, "Timed out waiting for flip_done\n");

Idea for a follow-up patch: Add something like drm_crtc_commit_wait which
skips on a NULL commit and does the two waits here. And use it here and in
drm_atomic_helper_wait_for_dependencies, we have four copies of the same
code by now :-)

> +	}
> +
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
>  	vc4_ctm_commit(vc4, state);
> @@ -368,6 +431,36 @@ static void commit_work(struct work_struct *work)
>  	vc4_atomic_complete_commit(state);
>  }
>  
> +static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct vc4_hvs_state *hvs_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +
> +	hvs_state = vc4_hvs_get_new_global_state(state);
> +	if (!hvs_state)
> +		return -EINVAL;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(crtc_state);
> +		unsigned int channel =
> +			vc4_crtc_state->assigned_channel;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		hvs_state->fifo_state[channel].pending_commit =
> +			drm_crtc_commit_get(crtc_state->commit);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * vc4_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -697,6 +790,7 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>  {
>  	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
>  	struct vc4_hvs_state *state;
> +	unsigned int i;
>  
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
>  	if (!state)
> @@ -706,6 +800,16 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>  
>  	state->unassigned_channels = old_state->unassigned_channels;
>  
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> +
> +		if (!old_state->fifo_state[i].pending_commit)
> +			continue;
> +
> +		state->fifo_state[i].pending_commit =
> +			drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
> +	}
> +
>  	return &state->base;
>  }
>  
> @@ -713,6 +817,14 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
>  					   struct drm_private_state *state)
>  {
>  	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
> +	unsigned int i;
> +
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		if (!hvs_state->fifo_state[i].pending_commit)
> +			continue;
> +
> +		drm_crtc_commit_put(hvs_state->fifo_state[i].pending_commit);
> +	}
>  
>  	kfree(hvs_state);
>  }
> @@ -805,7 +917,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  
>  		/* If we're disabling our CRTC, we put back our channel */
>  		if (!new_crtc_state->enable) {
> -			hvs_new_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			channel = old_vc4_crtc_state->assigned_channel;
> +
> +			hvs_new_state->unassigned_channels |= BIT(channel);
> +			hvs_new_state->fifo_state[channel].in_use = false;
>  			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>  			continue;
>  		}
> @@ -841,6 +956,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  		channel = ffs(matching_channels) - 1;
>  		new_vc4_crtc_state->assigned_channel = channel;
>  		hvs_new_state->unassigned_channels &= ~BIT(channel);
> +		hvs_new_state->fifo_state[channel].in_use = true;
>  	}
>  
>  	return 0;
> @@ -866,6 +982,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  	return vc4_load_tracker_atomic_check(state);
>  }
>  
> +static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
> +	.atomic_commit_setup	= vc4_atomic_commit_setup,
> +};
> +
>  static const struct drm_mode_config_funcs vc4_mode_funcs = {
>  	.atomic_check = vc4_atomic_check,
>  	.atomic_commit = vc4_atomic_commit,
> @@ -909,6 +1029,7 @@ int vc4_kms_load(struct drm_device *dev)
>  	}
>  
>  	dev->mode_config.funcs = &vc4_mode_funcs;
> +	dev->mode_config.helper_private = &vc4_mode_config_helpers;
>  	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.async_page_flip = true;
>  	dev->mode_config.allow_fb_modifiers = true;

Since I suggested this entire thing kinda:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.28.0
> 
> _______________________________________________
> 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

[PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

[Maxime Ripard]

The HVS state now has both unassigned_channels that reflects the
channels that are not used in the associated state, and the in_use
boolean for each channel that says whether or not a particular channel
is in use.

Both express pretty much the same thing, and we need the in_use variable
to properly track the commits, so let's get rid of unassigned_channels.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index fdd698df5fbe..fa40c44eb770 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -39,7 +39,6 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 
 struct vc4_hvs_state {
 	struct drm_private_state base;
-	unsigned int unassigned_channels;
 
 	struct {
 		unsigned in_use: 1;
@@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 
 	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
 
-	state->unassigned_channels = old_state->unassigned_channels;
 
 	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
 		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
@@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
 	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);
@@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 	struct vc4_hvs_state *hvs_new_state;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
+	unsigned int unassigned_channels;
 	unsigned int i;
 
 	hvs_new_state = vc4_hvs_get_global_state(state);
 	if (!hvs_new_state)
 		return -EINVAL;
 
+	for (i = 0; i < HVS_NUM_CHANNELS; i++)
+		if (!hvs_new_state->fifo_state[i].in_use)
+			unassigned_channels |= BIT(i);
+
 	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);
@@ -918,8 +920,6 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 		/* If we're disabling our CRTC, we put back our channel */
 		if (!new_crtc_state->enable) {
 			channel = old_vc4_crtc_state->assigned_channel;
-
-			hvs_new_state->unassigned_channels |= BIT(channel);
 			hvs_new_state->fifo_state[channel].in_use = false;
 			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
 			continue;
@@ -949,13 +949,13 @@ 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 = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
+		matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
 		if (!matching_channels)
 			return -EINVAL;
 
 		channel = ffs(matching_channels) - 1;
 		new_vc4_crtc_state->assigned_channel = channel;
-		hvs_new_state->unassigned_channels &= ~BIT(channel);
+		unassigned_channels &= ~BIT(channel);
 		hvs_new_state->fifo_state[channel].in_use = true;
 	}
 
-- 
2.28.0

Re: [PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

[kernel test robot]

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

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on linus/master v5.10-rc6 next-20201204]
[cannot apply to drm-intel/for-linux-next anholt/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/vc4-Convert-to-drm_atomic_helper_commit/20201204-231528
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: riscv-randconfig-r014-20201204 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 32c501dd88b62787d3a5ffda7aabcf4650dbe3cd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/1ac52fbc9e5e40633ecb194184b4ba69937e8b55
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maxime-Ripard/vc4-Convert-to-drm_atomic_helper_commit/20201204-231528
        git checkout 1ac52fbc9e5e40633ecb194184b4ba69937e8b55
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:556:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inb(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:55:76: note: expanded from macro 'inb'
   #define inb(c)          ({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:87:48: note: expanded from macro 'readb_cpu'
   #define readb_cpu(c)            ({ u8  __r = __raw_readb(c); __r; })
                                                            ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inw(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw'
   #define inw(c)          ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
   #define readw_cpu(c)            ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inl(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl'
   #define inl(c)          ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
   #define readl_cpu(c)            ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outb(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb'
   #define outb(v,c)       ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
   #define writeb_cpu(v, c)        ((void)__raw_writeb((v), (c)))
                                                             ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outw(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw'
   #define outw(v,c)       ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
   #define writew_cpu(v, c)        ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
                                                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outl(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl'
   #define outl(v,c)       ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
   #define writel_cpu(v, c)        ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
                                                                                     ^
   In file included from drivers/gpu/drm/vc4/vc4_kms.c:16:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:31:
   In file included from include/linux/fb.h:17:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/gpu/drm/vc4/vc4_kms.c:902:4: warning: variable 'unassigned_channels' is uninitialized when used here [-Wuninitialized]
                           unassigned_channels |= BIT(i);
                           ^~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vc4/vc4_kms.c:893:34: note: initialize the variable 'unassigned_channels' to silence this warning
           unsigned int unassigned_channels;
                                           ^
                                            = 0
   8 warnings generated.

vim +/unassigned_channels +902 drivers/gpu/drm/vc4/vc4_kms.c

   856	
   857	/*
   858	 * The BCM2711 HVS has up to 7 outputs connected to the pixelvalves and
   859	 * the TXP (and therefore all the CRTCs found on that platform).
   860	 *
   861	 * The naive (and our initial) implementation would just iterate over
   862	 * all the active CRTCs, try to find a suitable FIFO, and then remove it
   863	 * from the pool of available FIFOs. However, there are a few corner
   864	 * cases that need to be considered:
   865	 *
   866	 * - When running in a dual-display setup (so with two CRTCs involved),
   867	 *   we can update the state of a single CRTC (for example by changing
   868	 *   its mode using xrandr under X11) without affecting the other. In
   869	 *   this case, the other CRTC wouldn't be in the state at all, so we
   870	 *   need to consider all the running CRTCs in the DRM device to assign
   871	 *   a FIFO, not just the one in the state.
   872	 *
   873	 * - To fix the above, we can't use drm_atomic_get_crtc_state on all
   874	 *   enabled CRTCs to pull their CRTC state into the global state, since
   875	 *   a page flip would start considering their vblank to complete. Since
   876	 *   we don't have a guarantee that they are actually active, that
   877	 *   vblank might never happen, and shouldn't even be considered if we
   878	 *   want to do a page flip on a single CRTC. That can be tested by
   879	 *   doing a modetest -v first on HDMI1 and then on HDMI0.
   880	 *
   881	 * - Since we need the pixelvalve to be disabled and enabled back when
   882	 *   the FIFO is changed, we should keep the FIFO assigned for as long
   883	 *   as the CRTC is enabled, only considering it free again once that
   884	 *   CRTC has been disabled. This can be tested by booting X11 on a
   885	 *   single display, and changing the resolution down and then back up.
   886	 */
   887	static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
   888					      struct drm_atomic_state *state)
   889	{
   890		struct vc4_hvs_state *hvs_new_state;
   891		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
   892		struct drm_crtc *crtc;
   893		unsigned int unassigned_channels;
   894		unsigned int i;
   895	
   896		hvs_new_state = vc4_hvs_get_global_state(state);
   897		if (!hvs_new_state)
   898			return -EINVAL;
   899	
   900		for (i = 0; i < HVS_NUM_CHANNELS; i++)
   901			if (!hvs_new_state->fifo_state[i].in_use)
 > 902				unassigned_channels |= BIT(i);
   903	
   904		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
   905			struct vc4_crtc_state *old_vc4_crtc_state =
   906				to_vc4_crtc_state(old_crtc_state);
   907			struct vc4_crtc_state *new_vc4_crtc_state =
   908				to_vc4_crtc_state(new_crtc_state);
   909			struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
   910			unsigned int matching_channels;
   911			unsigned int channel;
   912	
   913			/* Nothing to do here, let's skip it */
   914			if (old_crtc_state->enable == new_crtc_state->enable)
   915				continue;
   916	
   917			/* Muxing will need to be modified, mark it as such */
   918			new_vc4_crtc_state->update_muxing = true;
   919	
   920			/* If we're disabling our CRTC, we put back our channel */
   921			if (!new_crtc_state->enable) {
   922				channel = old_vc4_crtc_state->assigned_channel;
   923				hvs_new_state->fifo_state[channel].in_use = false;
   924				new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
   925				continue;
   926			}
   927	
   928			/*
   929			 * The problem we have to solve here is that we have
   930			 * up to 7 encoders, connected to up to 6 CRTCs.
   931			 *
   932			 * Those CRTCs, depending on the instance, can be
   933			 * routed to 1, 2 or 3 HVS FIFOs, and we need to set
   934			 * the change the muxing between FIFOs and outputs in
   935			 * the HVS accordingly.
   936			 *
   937			 * It would be pretty hard to come up with an
   938			 * algorithm that would generically solve
   939			 * this. However, the current routing trees we support
   940			 * allow us to simplify a bit the problem.
   941			 *
   942			 * Indeed, with the current supported layouts, if we
   943			 * try to assign in the ascending crtc index order the
   944			 * FIFOs, we can't fall into the situation where an
   945			 * earlier CRTC that had multiple routes is assigned
   946			 * one that was the only option for a later CRTC.
   947			 *
   948			 * If the layout changes and doesn't give us that in
   949			 * the future, we will need to have something smarter,
   950			 * but it works so far.
   951			 */
   952			matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
   953			if (!matching_channels)
   954				return -EINVAL;
   955	
   956			channel = ffs(matching_channels) - 1;
   957			new_vc4_crtc_state->assigned_channel = channel;
   958			unassigned_channels &= ~BIT(channel);
   959			hvs_new_state->fifo_state[channel].in_use = true;
   960		}
   961	
   962		return 0;
   963	}
   964	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29635 bytes --]

Re: [PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

[Maxime Ripard]

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

On Fri, Dec 04, 2020 at 04:11:36PM +0100, Maxime Ripard wrote:
> @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  	struct vc4_hvs_state *hvs_new_state;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_crtc *crtc;
> +	unsigned int unassigned_channels;

This should be initialized to 0, I'll fix it up while applying if
there's no other comment.

Maxime

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

Re: [PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

[Thomas Zimmermann]

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

Hi

Am 04.12.20 um 16:11 schrieb Maxime Ripard:
> The HVS state now has both unassigned_channels that reflects the
> channels that are not used in the associated state, and the in_use
> boolean for each channel that says whether or not a particular channel
> is in use.
> 
> Both express pretty much the same thing, and we need the in_use variable
> to properly track the commits, so let's get rid of unassigned_channels.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index fdd698df5fbe..fa40c44eb770 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -39,7 +39,6 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>   
>   struct vc4_hvs_state {
>   	struct drm_private_state base;
> -	unsigned int unassigned_channels;
>   
>   	struct {
>   		unsigned in_use: 1;
> @@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>   
>   	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>   
> -	state->unassigned_channels = old_state->unassigned_channels;
>   
>   	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
>   		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> @@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
>   	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);
> @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   	struct vc4_hvs_state *hvs_new_state;
>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>   	struct drm_crtc *crtc;
> +	unsigned int unassigned_channels;
>   	unsigned int i;
>   
>   	hvs_new_state = vc4_hvs_get_global_state(state);
>   	if (!hvs_new_state)
>   		return -EINVAL;
>   
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++)
> +		if (!hvs_new_state->fifo_state[i].in_use)
> +			unassigned_channels |= BIT(i);
> +

More of a nit: I'd turn this block into a helper of struct 
vc4_hvs_state. That would also remove the need to initialize 
unassigned_channels to 0 here.

For the loop's condition, it might be less error prone to use 
ARRAY_SIZE(hvs_new_state->fifo_state) instead of HVS_NUM_CHANNEL.

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);
> @@ -918,8 +920,6 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   		/* If we're disabling our CRTC, we put back our channel */
>   		if (!new_crtc_state->enable) {
>   			channel = old_vc4_crtc_state->assigned_channel;
> -
> -			hvs_new_state->unassigned_channels |= BIT(channel);
>   			hvs_new_state->fifo_state[channel].in_use = false;
>   			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
> @@ -949,13 +949,13 @@ 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 = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
> +		matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;
>   		if (!matching_channels)
>   			return -EINVAL;
>   
>   		channel = ffs(matching_channels) - 1;
>   		new_vc4_crtc_state->assigned_channel = channel;
> -		hvs_new_state->unassigned_channels &= ~BIT(channel);
> +		unassigned_channels &= ~BIT(channel);
>   		hvs_new_state->fifo_state[channel].in_use = true;
>   	}
>   
> 

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

[PATCH v2 6/7] drm/vc4: kms: Remove async modeset semaphore

[Maxime Ripard]

Now that we have proper ordering guaranteed by the previous patch, the
semaphore is redundant and can be removed.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 -------------
 drivers/gpu/drm/vc4/vc4_drv.h  |  2 --
 drivers/gpu/drm/vc4/vc4_kms.c  | 24 ++----------------------
 3 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 482219fb4db2..c469594a2d3a 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -690,7 +690,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
 		container_of(cb, struct vc4_async_flip_state, cb);
 	struct drm_crtc *crtc = flip_state->crtc;
 	struct drm_device *dev = crtc->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct drm_plane *plane = crtc->primary;
 
 	vc4_plane_async_set_fb(plane, flip_state->fb);
@@ -722,8 +721,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb)
 	}
 
 	kfree(flip_state);
-
-	up(&vc4->async_modeset);
 }
 
 /* Implements async (non-vblank-synced) page flips.
@@ -738,7 +735,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
 			       uint32_t flags)
 {
 	struct drm_device *dev = crtc->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct drm_plane *plane = crtc->primary;
 	int ret = 0;
 	struct vc4_async_flip_state *flip_state;
@@ -767,15 +763,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc,
 	flip_state->crtc = crtc;
 	flip_state->event = event;
 
-	/* Make sure all other async modesetes have landed. */
-	ret = down_interruptible(&vc4->async_modeset);
-	if (ret) {
-		drm_framebuffer_put(fb);
-		vc4_bo_dec_usecnt(bo);
-		kfree(flip_state);
-		return ret;
-	}
-
 	/* Save the current FB before it's replaced by the new one in
 	 * drm_atomic_set_fb_for_plane(). We'll need the old FB in
 	 * vc4_async_page_flip_complete() to decrement the BO usecnt and keep
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index c5f2944d5bc6..4dcef3140dff 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -215,8 +215,6 @@ struct vc4_dev {
 		struct work_struct reset_work;
 	} hangcheck;
 
-	struct semaphore async_modeset;
-
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
 	struct drm_private_obj hvs_channels;
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index fa40c44eb770..ffbfdde55fff 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -418,8 +418,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 		clk_set_min_rate(hvs->core_clk, 0);
 
 	drm_atomic_state_put(state);
-
-	up(&vc4->async_modeset);
 }
 
 static void commit_work(struct work_struct *work)
@@ -477,26 +475,17 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock)
 {
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	int ret;
 
 	if (state->async_update) {
-		ret = down_interruptible(&vc4->async_modeset);
-		if (ret)
-			return ret;
-
 		ret = drm_atomic_helper_prepare_planes(dev, state);
-		if (ret) {
-			up(&vc4->async_modeset);
+		if (ret)
 			return ret;
-		}
 
 		drm_atomic_helper_async_commit(dev, state);
 
 		drm_atomic_helper_cleanup_planes(dev, state);
 
-		up(&vc4->async_modeset);
-
 		return 0;
 	}
 
@@ -512,21 +501,14 @@ static int vc4_atomic_commit(struct drm_device *dev,
 
 	INIT_WORK(&state->commit_work, commit_work);
 
-	ret = down_interruptible(&vc4->async_modeset);
-	if (ret)
-		return ret;
-
 	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret) {
-		up(&vc4->async_modeset);
+	if (ret)
 		return ret;
-	}
 
 	if (!nonblock) {
 		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
 		if (ret) {
 			drm_atomic_helper_cleanup_planes(dev, state);
-			up(&vc4->async_modeset);
 			return ret;
 		}
 	}
@@ -1008,8 +990,6 @@ int vc4_kms_load(struct drm_device *dev)
 		vc4->load_tracker_enabled = true;
 	}
 
-	sema_init(&vc4->async_modeset, 1);
-
 	/* Set support for vblank irq fast disable, before drm_vblank_init() */
 	dev->vblank_disable_immediate = true;
 
-- 
2.28.0

[PATCH v2 7/7] drm/vc4: kms: Convert to atomic helpers

[Maxime Ripard]

Now that the semaphore is gone, our atomic_commit implementation is
basically drm_atomic_helper_commit with a somewhat custom commit_tail,
the main difference being that we're using wait_for_flip_done instead of
wait_for_vblanks used in the drm_atomic_helper_commit_tail helper.

Let's switch to using drm_atomic_helper_commit.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 110 +---------------------------------
 1 file changed, 3 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ffbfdde55fff..05f451f3e642 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -332,8 +332,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4,
 	}
 }
 
-static void
-vc4_atomic_complete_commit(struct drm_atomic_state *state)
+static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
@@ -357,10 +356,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 	if (vc4->hvs->hvs5)
 		clk_set_min_rate(hvs->core_clk, 500000000);
 
-	drm_atomic_helper_wait_for_fences(dev, state, false);
-
-	drm_atomic_helper_wait_for_dependencies(state);
-
 	old_hvs_state = vc4_hvs_get_old_global_state(state);
 	if (!old_hvs_state)
 		return;
@@ -412,20 +407,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 
-	drm_atomic_helper_commit_cleanup_done(state);
-
 	if (vc4->hvs->hvs5)
 		clk_set_min_rate(hvs->core_clk, 0);
-
-	drm_atomic_state_put(state);
-}
-
-static void commit_work(struct work_struct *work)
-{
-	struct drm_atomic_state *state = container_of(work,
-						      struct drm_atomic_state,
-						      commit_work);
-	vc4_atomic_complete_commit(state);
 }
 
 static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
@@ -458,94 +441,6 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
 	return 0;
 }
 
-/**
- * vc4_atomic_commit - commit validated state object
- * @dev: DRM device
- * @state: the driver state object
- * @nonblock: nonblocking commit
- *
- * This function commits a with drm_atomic_helper_check() pre-validated state
- * object. This can still fail when e.g. the framebuffer reservation fails. For
- * now this doesn't implement asynchronous commits.
- *
- * RETURNS
- * Zero for success or -errno.
- */
-static int vc4_atomic_commit(struct drm_device *dev,
-			     struct drm_atomic_state *state,
-			     bool nonblock)
-{
-	int ret;
-
-	if (state->async_update) {
-		ret = drm_atomic_helper_prepare_planes(dev, state);
-		if (ret)
-			return ret;
-
-		drm_atomic_helper_async_commit(dev, state);
-
-		drm_atomic_helper_cleanup_planes(dev, state);
-
-		return 0;
-	}
-
-	/* We know for sure we don't want an async update here. Set
-	 * state->legacy_cursor_update to false to prevent
-	 * drm_atomic_helper_setup_commit() from auto-completing
-	 * commit->flip_done.
-	 */
-	state->legacy_cursor_update = false;
-	ret = drm_atomic_helper_setup_commit(state, nonblock);
-	if (ret)
-		return ret;
-
-	INIT_WORK(&state->commit_work, commit_work);
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	if (!nonblock) {
-		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
-		if (ret) {
-			drm_atomic_helper_cleanup_planes(dev, state);
-			return ret;
-		}
-	}
-
-	/*
-	 * This is the point of no return - everything below never fails except
-	 * when the hw goes bonghits. Which means we can commit the new state on
-	 * the software side now.
-	 */
-
-	BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
-
-	/*
-	 * Everything below can be run asynchronously without the need to grab
-	 * any modeset locks at all under one condition: It must be guaranteed
-	 * that the asynchronous work has either been cancelled (if the driver
-	 * supports it, which at least requires that the framebuffers get
-	 * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
-	 * before the new state gets committed on the software side with
-	 * drm_atomic_helper_swap_state().
-	 *
-	 * This scheme allows new atomic state updates to be prepared and
-	 * checked in parallel to the asynchronous completion of the previous
-	 * update. Which is important since compositors need to figure out the
-	 * composition of the next frame right after having submitted the
-	 * current layout.
-	 */
-
-	drm_atomic_state_get(state);
-	if (nonblock)
-		queue_work(system_unbound_wq, &state->commit_work);
-	else
-		vc4_atomic_complete_commit(state);
-
-	return 0;
-}
-
 static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
 					     struct drm_file *file_priv,
 					     const struct drm_mode_fb_cmd2 *mode_cmd)
@@ -966,11 +861,12 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 
 static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
 	.atomic_commit_setup	= vc4_atomic_commit_setup,
+	.atomic_commit_tail	= vc4_atomic_commit_tail,
 };
 
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.atomic_check = vc4_atomic_check,
-	.atomic_commit = vc4_atomic_commit,
+	.atomic_commit = drm_atomic_helper_commit,
 	.fb_create = vc4_fb_create,
 };
 
-- 
2.28.0

Re: [PATCH v2 0/7] vc4: Convert to drm_atomic_helper_commit

[Maxime Ripard]

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

On Fri, Dec 04, 2020 at 04:11:31PM +0100, Maxime Ripard wrote:
> Hi,
> 
> Here's a conversion of vc4 to remove the hand-rolled atomic_commit helper from
> vc4 in favour of the generic one.
> 
> This requires some rework of vc4, but also a new hook and some documentation
> for corner-cases in the DRM core that have been reported and explained by
> Daniel recently.
> 
> Let me know what you think,
> Maxime

Applied, thanks!
Maxime

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