[PATCH 4/5] drm/msm/mdp5: dynamically assign hw pipes to planes
Hi,
Minor comments below. LGTM otherwise.
On 11/05/2016 09:56 PM, Rob Clark wrote:
> (re)assign the hw pipes to planes based on required caps, and to handle
> situations where we could not modify an in-use plane (ie. SMP block
> reallocation).
>
> This means all planes advertise the superset of formats and properties.
> Userspace must (as always) use atomic TEST_ONLY step for atomic updates,
> as not all planes may be available for use on every frame.
>
> The mapping of hwpipe to plane is stored in mdp5_state, so that state
> updates are atomically committed in the same way that plane/etc state
> updates are managed. This is needed because the mdp5_plane_state keeps
> a pointer to the hwpipe, and we don't want global state to become out
> of sync with the plane state if an atomic update fails, we hit deadlock/
> backoff scenario, etc. The use of state_lock keeps multiple parallel
> updates which both re-assign hwpipes properly serialized.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 4 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 7 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c | 70 +
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h | 10 ++
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 158
> --
> 6 files changed, 171 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index 6213f51..99958f7 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -407,7 +407,7 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> for (i = 0; i < cnt; i++) {
> pstates[i].state->stage = STAGE_BASE + i + base;
> DBG("%s: assign pipe %s on stage=%d", crtc->name,
> - pipe2name(mdp5_plane_pipe(pstates[i].plane)),
> + pstates[i].plane->name,
> pstates[i].state->stage);
> }
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index ca6dfeb..3542adf 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -92,7 +92,7 @@ struct mdp5_state *mdp5_get_state(struct drm_atomic_state
> *s)
> return ERR_PTR(-ENOMEM);
>
> /* Copy state: */
> - /* TODO */
> + new_state->hwpipe = mdp5_kms->state->hwpipe;
>
> state->state = new_state;
>
> @@ -377,7 +377,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> struct drm_plane *plane;
> struct drm_crtc *crtc;
>
> - plane = mdp5_plane_init(dev, mdp5_kms->hwpipes[i], primary);
> + plane = mdp5_plane_init(dev, primary);
> if (IS_ERR(plane)) {
> ret = PTR_ERR(plane);
> dev_err(dev->dev, "failed to construct plane %d
> (%d)\n", i, ret);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index 52914ec..4475090 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -82,7 +82,7 @@ struct mdp5_kms {
> * For atomic updates which require modifying global state,
> */
> struct mdp5_state {
> - uint32_t dummy;
> + struct mdp5_hw_pipe_state hwpipe;
I was wondering if we could rename the above as hwpipes/hwpipe_state/hwpipe_map,
because it gets a confusing since variables of the type mdp5_hw_pipe are
also named hwpipe.
> };
>
> struct mdp5_state *__must_check
> @@ -94,6 +94,8 @@ mdp5_get_state(struct drm_atomic_state *s);
> struct mdp5_plane_state {
> struct drm_plane_state base;
>
> + struct mdp5_hw_pipe *hwpipe;
> +
> /* aligned with property */
> uint8_t premultiplied;
> uint8_t zpos;
> @@ -232,8 +234,7 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane);
> void mdp5_plane_complete_commit(struct drm_plane *plane,
> struct drm_plane_state *state);
> enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane);
> -struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> - struct mdp5_hw_pipe *hwpipe, bool primary);
> +struct drm_plane *mdp5_plane_init(struct drm_device *dev, bool primary);
>
> uint32_t mdp5_crtc_vblank(struct drm_crtc *crtc);
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> index 7f3e8e50..115de7d 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
> @@ -17,6 +17,76 @@
>
> #include "mdp5_kms.h"
>
> +struct mdp5_hw_pipe *mdp5_pipe_assign(struct drm_atomic_state *s,
> + struct drm_plane *plane, uint32_t caps)
> +{
> + struct msm_drm_private *priv = s->dev->dev_private;
> + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> +
[PATCH 4/5] drm/msm/mdp5: dynamically assign hw pipes to planes
(re)assign the hw pipes to planes based on required caps, and to handle
situations where we could not modify an in-use plane (ie. SMP block
reallocation).
This means all planes advertise the superset of formats and properties.
Userspace must (as always) use atomic TEST_ONLY step for atomic updates,
as not all planes may be available for use on every frame.
The mapping of hwpipe to plane is stored in mdp5_state, so that state
updates are atomically committed in the same way that plane/etc state
updates are managed. This is needed because the mdp5_plane_state keeps
a pointer to the hwpipe, and we don't want global state to become out
of sync with the plane state if an atomic update fails, we hit deadlock/
backoff scenario, etc. The use of state_lock keeps multiple parallel
updates which both re-assign hwpipes properly serialized.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 4 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 7 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c | 70 +
drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.h | 10 ++
drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 158 --
6 files changed, 171 insertions(+), 80 deletions(-)
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
index 6213f51..99958f7 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
@@ -407,7 +407,7 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
for (i = 0; i < cnt; i++) {
pstates[i].state->stage = STAGE_BASE + i + base;
DBG("%s: assign pipe %s on stage=%d", crtc->name,
- pipe2name(mdp5_plane_pipe(pstates[i].plane)),
+ pstates[i].plane->name,
pstates[i].state->stage);
}
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index ca6dfeb..3542adf 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -92,7 +92,7 @@ struct mdp5_state *mdp5_get_state(struct drm_atomic_state *s)
return ERR_PTR(-ENOMEM);
/* Copy state: */
- /* TODO */
+ new_state->hwpipe = mdp5_kms->state->hwpipe;
state->state = new_state;
@@ -377,7 +377,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
struct drm_plane *plane;
struct drm_crtc *crtc;
- plane = mdp5_plane_init(dev, mdp5_kms->hwpipes[i], primary);
+ plane = mdp5_plane_init(dev, primary);
if (IS_ERR(plane)) {
ret = PTR_ERR(plane);
dev_err(dev->dev, "failed to construct plane %d
(%d)\n", i, ret);
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
index 52914ec..4475090 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
@@ -82,7 +82,7 @@ struct mdp5_kms {
* For atomic updates which require modifying global state,
*/
struct mdp5_state {
- uint32_t dummy;
+ struct mdp5_hw_pipe_state hwpipe;
};
struct mdp5_state *__must_check
@@ -94,6 +94,8 @@ mdp5_get_state(struct drm_atomic_state *s);
struct mdp5_plane_state {
struct drm_plane_state base;
+ struct mdp5_hw_pipe *hwpipe;
+
/* aligned with property */
uint8_t premultiplied;
uint8_t zpos;
@@ -232,8 +234,7 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane);
void mdp5_plane_complete_commit(struct drm_plane *plane,
struct drm_plane_state *state);
enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane);
-struct drm_plane *mdp5_plane_init(struct drm_device *dev,
- struct mdp5_hw_pipe *hwpipe, bool primary);
+struct drm_plane *mdp5_plane_init(struct drm_device *dev, bool primary);
uint32_t mdp5_crtc_vblank(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
index 7f3e8e50..115de7d 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_pipe.c
@@ -17,6 +17,76 @@
#include "mdp5_kms.h"
+struct mdp5_hw_pipe *mdp5_pipe_assign(struct drm_atomic_state *s,
+ struct drm_plane *plane, uint32_t caps)
+{
+ struct msm_drm_private *priv = s->dev->dev_private;
+ struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
+ struct mdp5_state *state = mdp5_get_state(s);
+ struct mdp5_hw_pipe_state *old_state, *new_state;
+ struct mdp5_hw_pipe *hwpipe = NULL;
+ int i;
+
+ if (IS_ERR(state))
+ return ERR_CAST(state);
+
+ /* grab old_state after mdp5_get_state(), since now we hold lock: */
+ old_state = &mdp5_kms->state->hwpipe;
+ new_state = &state->hwpipe;
+
+ for
