Re: [RFC 3/3] drm/omap: Add wide display support using multiple overlays
Hi Benoit,
On 29/06/18 20:02, Benoit Parrot wrote:
> In the case where we need to support wide-display using more than one
> overlay to achieve the needed display resolution, we need to be able to
> dynamically assign overlays to planes instead of having the overlays
> being statically mapped to planes.
>
> This also means that on occasion where the number of requested planes
> exceeds the numbers of overlays required to display them then a failure
> would be returned for the plane that cannot be handled at that time. It
> is up to user space to make sure the H/W resource are not
> over-subscribed.
This is not a trivial patch, I think it needs much more explanation in
the desc on how the patch solves this issue and what it changes.
I have some small comments below here and there, but overall, I feel
it's rather difficult to review, and I really think this should be split
into multiple smaller patches. Some ideas on the patches it might be
split to:
- add plane state
- add global state
- add code which manages plane-id -> hw plane id
- add dual plane support
> Signed-off-by: Benoit Parrot
> ---
> drivers/gpu/drm/omapdrm/Makefile | 1 +
> drivers/gpu/drm/omapdrm/omap_drv.c | 119 ++-
> drivers/gpu/drm/omapdrm/omap_drv.h | 15 +-
> drivers/gpu/drm/omapdrm/omap_fb.c | 33 ++-
> drivers/gpu/drm/omapdrm/omap_fb.h | 4 +-
> drivers/gpu/drm/omapdrm/omap_overlay.c | 366
> +
> drivers/gpu/drm/omapdrm/omap_overlay.h | 80 +++
> drivers/gpu/drm/omapdrm/omap_plane.c | 322 -
> drivers/gpu/drm/omapdrm/omap_plane.h | 20 ++
> 9 files changed, 892 insertions(+), 68 deletions(-)
> create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c
> create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h
>
> diff --git a/drivers/gpu/drm/omapdrm/Makefile
> b/drivers/gpu/drm/omapdrm/Makefile
> index f115253115c5..800dfd035360 100644
> --- a/drivers/gpu/drm/omapdrm/Makefile
> +++ b/drivers/gpu/drm/omapdrm/Makefile
> @@ -12,6 +12,7 @@ omapdrm-y := omap_drv.o \
> omap_debugfs.o \
> omap_crtc.o \
> omap_plane.o \
> + omap_overlay.o \
> omap_encoder.o \
> omap_connector.o \
> omap_fb.o \
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index ef3b0e3571ec..f0a5c3dab471 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -16,11 +16,7 @@
> */
>
> #include
> -
> -#include
> -#include
> -#include
> -#include
> +#include
>
> #include "omap_dmm_tiler.h"
> #include "omap_drv.h"
> @@ -113,9 +109,100 @@ static void omap_atomic_commit_tail(struct
> drm_atomic_state *old_state)
>
> drm_atomic_helper_cleanup_planes(dev, old_state);
>
> + omap_overlay_disable_unassigned(old_state);
> +
> priv->dispc_ops->runtime_put(priv->dispc);
> }
>
> +static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
> +{
> + const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
> + const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
> +
> + if (sa->normalized_zpos != sb->normalized_zpos)
> + return sa->normalized_zpos - sb->normalized_zpos;
> + else
> + return sa->plane->base.id - sb->plane->base.id;
> +}
> +
> +static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_state, *new_state;
> + struct drm_plane *plane;
> + int i, n, inc;
> + int total_planes = dev->mode_config.num_total_plane;
> + struct drm_plane_state **states;
> + int ret = 0;
> +
> + states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
> + if (!states)
> + return -ENOMEM;
> +
> + for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, i) {
> + if (old_state->plane_mask == new_state->plane_mask &&
> + !new_state->zpos_changed)
> + continue;
> +
> + /* Reset plane increment and index value for every crtc */
> + n = 0;
> +
> + /*
> + * Normalization process might create new states for planes
> + * which normalized_zpos has to be recalculated.
> + */
> + drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_plane_state(new_state->state,
> +plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto done;
> + }
> + states[n++] = plane_state;
> + }
> +
> + sort(
[RFC 3/3] drm/omap: Add wide display support using multiple overlays
In the case where we need to support wide-display using more than one
overlay to achieve the needed display resolution, we need to be able to
dynamically assign overlays to planes instead of having the overlays
being statically mapped to planes.
This also means that on occasion where the number of requested planes
exceeds the numbers of overlays required to display them then a failure
would be returned for the plane that cannot be handled at that time. It
is up to user space to make sure the H/W resource are not
over-subscribed.
Signed-off-by: Benoit Parrot
---
drivers/gpu/drm/omapdrm/Makefile | 1 +
drivers/gpu/drm/omapdrm/omap_drv.c | 119 ++-
drivers/gpu/drm/omapdrm/omap_drv.h | 15 +-
drivers/gpu/drm/omapdrm/omap_fb.c | 33 ++-
drivers/gpu/drm/omapdrm/omap_fb.h | 4 +-
drivers/gpu/drm/omapdrm/omap_overlay.c | 366 +
drivers/gpu/drm/omapdrm/omap_overlay.h | 80 +++
drivers/gpu/drm/omapdrm/omap_plane.c | 322 -
drivers/gpu/drm/omapdrm/omap_plane.h | 20 ++
9 files changed, 892 insertions(+), 68 deletions(-)
create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c
create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h
diff --git a/drivers/gpu/drm/omapdrm/Makefile b/drivers/gpu/drm/omapdrm/Makefile
index f115253115c5..800dfd035360 100644
--- a/drivers/gpu/drm/omapdrm/Makefile
+++ b/drivers/gpu/drm/omapdrm/Makefile
@@ -12,6 +12,7 @@ omapdrm-y := omap_drv.o \
omap_debugfs.o \
omap_crtc.o \
omap_plane.o \
+ omap_overlay.o \
omap_encoder.o \
omap_connector.o \
omap_fb.o \
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
b/drivers/gpu/drm/omapdrm/omap_drv.c
index ef3b0e3571ec..f0a5c3dab471 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -16,11 +16,7 @@
*/
#include
-
-#include
-#include
-#include
-#include
+#include
#include "omap_dmm_tiler.h"
#include "omap_drv.h"
@@ -113,9 +109,100 @@ static void omap_atomic_commit_tail(struct
drm_atomic_state *old_state)
drm_atomic_helper_cleanup_planes(dev, old_state);
+ omap_overlay_disable_unassigned(old_state);
+
priv->dispc_ops->runtime_put(priv->dispc);
}
+static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
+{
+ const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
+ const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
+
+ if (sa->normalized_zpos != sb->normalized_zpos)
+ return sa->normalized_zpos - sb->normalized_zpos;
+ else
+ return sa->plane->base.id - sb->plane->base.id;
+}
+
+static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
+struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_state, *new_state;
+ struct drm_plane *plane;
+ int i, n, inc;
+ int total_planes = dev->mode_config.num_total_plane;
+ struct drm_plane_state **states;
+ int ret = 0;
+
+ states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
+ if (!states)
+ return -ENOMEM;
+
+ for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, i) {
+ if (old_state->plane_mask == new_state->plane_mask &&
+ !new_state->zpos_changed)
+ continue;
+
+ /* Reset plane increment and index value for every crtc */
+ n = 0;
+
+ /*
+* Normalization process might create new states for planes
+* which normalized_zpos has to be recalculated.
+*/
+ drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(new_state->state,
+ plane);
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto done;
+ }
+ states[n++] = plane_state;
+ }
+
+ sort(states, n, sizeof(*states),
+drm_atomic_state_normalized_zpos_cmp, NULL);
+
+ for (i = 0, inc = 0; i < n; i++) {
+ plane = states[i]->plane;
+
+ states[i]->normalized_zpos = i + inc;
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos
value %d\n",
+plane->base.id, plane->name,
+states[i]->normalized_zpos);
+
+ if (is_omap_plane_dual_overlay(states[i]))
+ inc++;
+ }
+ new_state->zpos_changed =
