Re: [PATCH v5 09/10] drm: rcar-du: Perform group setup from the atomic tail handler

2021-03-26 Thread Maxime Ripard
Hi Kieran,

On Mon, Mar 22, 2021 at 04:35:34PM +, Kieran Bingham wrote:
> Create rcar_du_group_atomic_check() and rcar_du_group_atomic_setup()
> functions to track and apply group state through the DRM atomic state.
> The use_count field is moved from the rcar_du_group structure to an
> enabled field in the rcar_du_group_state structure.
> 
> This allows separating group setup from the configuration of the CRTCs
> that are part of the group, simplifying the CRTC code and improving
> overall readability. The existing rcar_du_group_{get,put}() functions
> are now redundant and removed.
> 
> The groups share clocking with the CRTCs within the group, and so are
> accessible only when one of its CRTCs has been powered through
> rcar_du_crtc_atomic_exit_standby().
> 
> Reviewed-by: Ulrich Hecht 
> Signed-off-by: Kieran Bingham 
> Signed-off-by: Laurent Pinchart 

It's a bit weird to have both your and Laurent's SoB without a
Co-Developped-By or an authorship from him.

However, using a drm_private_obj shared between CRTC has a gotcha: you
don't have any ordering guarantee between commits if they affect
different CRTCs (and they are non-blocking).

Let's assume we have two subsequent commits, commit1 and commit2, both
non-blocking, and affecting different CRTC, plane and connectors. In
this case, commit1 old private state will be commit0 new private state,
and commit 2 old private state will be commit1 new private state.

If commit2 is executed before commit1, then it will free its old state
when done with it (so commit1 new private state), and will thus result
in an use-after-free when commit1 is ran.

In order to fix this, you should store (and get a reference to) the
drm_crtc_commit in your private state in atomic_commit_setup, and call
drm_crtc_commit_wait on that commit as the first part of your
commit_tail to serialize those commits.

Maxime


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH v5 09/10] drm: rcar-du: Perform group setup from the atomic tail handler

2021-03-22 Thread Kieran Bingham
Create rcar_du_group_atomic_check() and rcar_du_group_atomic_setup()
functions to track and apply group state through the DRM atomic state.
The use_count field is moved from the rcar_du_group structure to an
enabled field in the rcar_du_group_state structure.

This allows separating group setup from the configuration of the CRTCs
that are part of the group, simplifying the CRTC code and improving
overall readability. The existing rcar_du_group_{get,put}() functions
are now redundant and removed.

The groups share clocking with the CRTCs within the group, and so are
accessible only when one of its CRTCs has been powered through
rcar_du_crtc_atomic_exit_standby().

Reviewed-by: Ulrich Hecht 
Signed-off-by: Kieran Bingham 
Signed-off-by: Laurent Pinchart 
---
Changes since v3:
- Shorted macro length, and added relevant documentation to its usage
  and function.

Changes since v2:

- Simplify error handling in rcar_du_crtc_enable()
- Rename rcar_du_group_atomic_pre_commit() to
  rcar_du_group_atomic_setup() and turn it into a void function
- Remove rcar_du_group_atomic_post_commit()
- Replace group state use_count field by enabled
- Rename group state variable from rstate to gstate

Changes since v1:

- All register sequences now maintained.
- Clock management is no longer handled by the group
  (_crtc_{exit,enter}_standby handles this for us)
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  18 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 102 
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  12 ++-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |   5 ++
 4 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 7ca721e6b9d7..020eb75732a7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -528,12 +528,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc 
*rcrtc)
return ret;
 
ret = clk_prepare_enable(rcrtc->extclock);
-   if (ret < 0)
-   goto error_clock;
-
-   ret = rcar_du_group_get(rcrtc->group);
-   if (ret < 0)
-   goto error_group;
+   if (ret < 0) {
+   clk_disable_unprepare(rcrtc->clock);
+   return ret;
+   }
 
/* Set display off and background to black. */
rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
@@ -543,18 +541,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc 
*rcrtc)
rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
return 0;
-
-error_group:
-   clk_disable_unprepare(rcrtc->extclock);
-error_clock:
-   clk_disable_unprepare(rcrtc->clock);
-   return ret;
 }
 
 static void rcar_du_crtc_enter_standby(struct rcar_du_crtc *rcrtc)
 {
-   rcar_du_group_put(rcrtc->group);
-
clk_disable_unprepare(rcrtc->extclock);
clk_disable_unprepare(rcrtc->clock);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c 
b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 7c13def703b7..fdd4a60f5f5e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -24,6 +24,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 
@@ -183,38 +184,6 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
mutex_unlock(>lock);
 }
 
-/*
- * rcar_du_group_get - Acquire a reference to the DU channels group
- *
- * Acquiring the first reference setups core registers. A reference must be 
held
- * before accessing any hardware registers.
- *
- * This function must be called with the DRM mode_config lock held.
- *
- * Return 0 in case of success or a negative error code otherwise.
- */
-int rcar_du_group_get(struct rcar_du_group *rgrp)
-{
-   if (rgrp->use_count)
-   goto done;
-
-   rcar_du_group_setup(rgrp);
-
-done:
-   rgrp->use_count++;
-   return 0;
-}
-
-/*
- * rcar_du_group_put - Release a reference to the DU
- *
- * This function must be called with the DRM mode_config lock held.
- */
-void rcar_du_group_put(struct rcar_du_group *rgrp)
-{
-   --rgrp->use_count;
-}
-
 static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
 {
struct rcar_du_device *rcdu = rgrp->dev;
@@ -399,6 +368,34 @@ static const struct drm_private_state_funcs 
rcar_du_group_state_funcs = {
.atomic_destroy_state = rcar_du_group_atomic_destroy_state,
 };
 
+/**
+ * for_each_oldnew_group_in_state - iterate over all groups in an atomic update
+ * @__state:  drm_atomic_state pointer
+ * @__obj:  drm_private_obj iteration cursor
+ * @__old:  drm_private_state iteration cursor for the old state
+ * @__new:  drm_private_state iteration cursor for the new state
+ * @__i: unsigned int iteration cursor, for macro-internal use
+ *
+ * Iterate over all private objects in an atomic update, processing only those
+ * which represent rcar_du_group_state, tracking both old and new