Re: [PATCH] drm/komeda: Adds zorder support
On Tue, Oct 08, 2019 at 05:00:46PM +0200, Daniel Vetter wrote:
> On Mon, May 20, 2019 at 5:33 AM Lowry Li (Arm Technology China)
> wrote:
> >
> > - Creates the zpos property.
> > - Implement komeda_crtc_normalize_zpos to replace
> > drm_atomic_normalize_zpos, reasons as the following:
> >
> > 1. The drm_atomic_normalize_zpos allows to configure same zpos for
> > different planes, but komeda doesn't support such configuration.
>
> Just stumbled over your custom normalized_zpos calculation, and it
> looks very fishy. You seem to reinvent the normalized zpos
> computation, which has the entire job of resolving duplicated zpos
> values (which can happen with legacy kms). So the above is definitely
> wrong.
>
> Can you pls do a patch to remove your own code, and replace it with
> helper usage? Or at least explain why exactly you can't use the
> standard normalized zpos stuff and need your own (since it really
> looks like just reinventing the same thing).
> -Daniel
>
> > 2. For further slave pipline case, Komeda need to calculate the
> > max_slave_zorder, we will merge such calculation into
> > komed_crtc_normalize_zpos to save a separated plane_state loop.
> > 3. For feature none-scaling layer_split, which a plane_state will be
> > assigned to two individual layers(left/right), which requires two
> > normalize_zpos for this plane, plane_st->normalize_zpos will be used
> > by left layer, normalize_zpos + 1 for right_layer.
> >
Yes, the komeda/drm zpos_normalize are simlilar,
First requirement is here the "layer split" support.
Komeda HW has a input size limitation for YUV format, compare with
RGB, which only has the half capablities, like
- Layer support 4K RGB input size.
- YUV only can support 2K.
To hide this limitation, we introdue Layer split, which splits
one drm_plane data flows to two halves (left/right), and handle them by
two individual HW layers, and of course, two layers need two different
normalize_zpos. but drm_atomic_normalize_zpos() only assign one value
to this plane, but we need two values for layer_split.
Secondary requirement:
we also need the ordered plane list that built by normalize_zpos(),
but current drm version doesn't return it.
For more komeda things:
https://www.kernel.org/doc/html/latest/gpu/komeda-kms.html?highlight=komeda
LAST.
Yes, the komeda and drm version zpos normalize are similar,
we can not use the standard version only because some tiny but
special requirements.
I saw the upstream is working for the zorder refinement, maybe we
can add komeda's requirements into the drm_atomic_normalize_zpos()
and drop komeda version together with this refinement.
Best Regards
James
> > This patch series depends on:
> > - https://patchwork.freedesktop.org/series/58710/
> > - https://patchwork.freedesktop.org/series/59000/
> > - https://patchwork.freedesktop.org/series/59002/
> > - https://patchwork.freedesktop.org/series/59747/
> > - https://patchwork.freedesktop.org/series/59915/
> > - https://patchwork.freedesktop.org/series/60083/
> > - https://patchwork.freedesktop.org/series/60698/
> >
> > Signed-off-by: Lowry Li (Arm Technology China)
> > ---
> > drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 90
> > ++-
> > drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
> > 3 files changed, 97 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 306ea06..0ec7665 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -100,6 +100,90 @@ static void komeda_kms_commit_tail(struct
> > drm_atomic_state *old_state)
> > .atomic_commit_tail = komeda_kms_commit_tail,
> > };
> >
> > +static int komeda_plane_state_list_add(struct drm_plane_state *plane_st,
> > + struct list_head *zorder_list)
> > +{
> > + struct komeda_plane_state *new = to_kplane_st(plane_st);
> > + struct komeda_plane_state *node, *last;
> > +
> > + last = list_empty(zorder_list) ?
> > + NULL : list_last_entry(zorder_list, typeof(*last),
> > zlist_node);
> > +
> > + /* Considering the list sequence is zpos increasing, so if list is
> > empty
> > +* or the zpos of new node bigger than the last node in list, no
> > need
> > +* loop and just insert the new one to the tail of the list.
> > +*/
> > + if (!last || (new->base.zpos > last->base.zpos)) {
> > + list_add_tail(>zlist_node, zorder_list);
> > + return 0;
> > + }
> > +
> > + /* Build the list by zpos increasing */
> > + list_for_each_entry(node, zorder_list, zlist_node) {
> > + if (new->base.zpos < node->base.zpos) {
> > + list_add_tail(>zlist_node, >zlist_node);
> > +
Re: [PATCH] drm/komeda: Adds zorder support
On Mon, May 20, 2019 at 5:33 AM Lowry Li (Arm Technology China)
wrote:
>
> - Creates the zpos property.
> - Implement komeda_crtc_normalize_zpos to replace
> drm_atomic_normalize_zpos, reasons as the following:
>
> 1. The drm_atomic_normalize_zpos allows to configure same zpos for
> different planes, but komeda doesn't support such configuration.
Just stumbled over your custom normalized_zpos calculation, and it
looks very fishy. You seem to reinvent the normalized zpos
computation, which has the entire job of resolving duplicated zpos
values (which can happen with legacy kms). So the above is definitely
wrong.
Can you pls do a patch to remove your own code, and replace it with
helper usage? Or at least explain why exactly you can't use the
standard normalized zpos stuff and need your own (since it really
looks like just reinventing the same thing).
-Daniel
> 2. For further slave pipline case, Komeda need to calculate the
> max_slave_zorder, we will merge such calculation into
> komed_crtc_normalize_zpos to save a separated plane_state loop.
> 3. For feature none-scaling layer_split, which a plane_state will be
> assigned to two individual layers(left/right), which requires two
> normalize_zpos for this plane, plane_st->normalize_zpos will be used
> by left layer, normalize_zpos + 1 for right_layer.
>
> This patch series depends on:
> - https://patchwork.freedesktop.org/series/58710/
> - https://patchwork.freedesktop.org/series/59000/
> - https://patchwork.freedesktop.org/series/59002/
> - https://patchwork.freedesktop.org/series/59747/
> - https://patchwork.freedesktop.org/series/59915/
> - https://patchwork.freedesktop.org/series/60083/
> - https://patchwork.freedesktop.org/series/60698/
>
> Signed-off-by: Lowry Li (Arm Technology China)
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 90
> ++-
> drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
> 3 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 306ea06..0ec7665 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -100,6 +100,90 @@ static void komeda_kms_commit_tail(struct
> drm_atomic_state *old_state)
> .atomic_commit_tail = komeda_kms_commit_tail,
> };
>
> +static int komeda_plane_state_list_add(struct drm_plane_state *plane_st,
> + struct list_head *zorder_list)
> +{
> + struct komeda_plane_state *new = to_kplane_st(plane_st);
> + struct komeda_plane_state *node, *last;
> +
> + last = list_empty(zorder_list) ?
> + NULL : list_last_entry(zorder_list, typeof(*last), zlist_node);
> +
> + /* Considering the list sequence is zpos increasing, so if list is
> empty
> +* or the zpos of new node bigger than the last node in list, no need
> +* loop and just insert the new one to the tail of the list.
> +*/
> + if (!last || (new->base.zpos > last->base.zpos)) {
> + list_add_tail(>zlist_node, zorder_list);
> + return 0;
> + }
> +
> + /* Build the list by zpos increasing */
> + list_for_each_entry(node, zorder_list, zlist_node) {
> + if (new->base.zpos < node->base.zpos) {
> + list_add_tail(>zlist_node, >zlist_node);
> + break;
> + } else if (node->base.zpos == new->base.zpos) {
> + struct drm_plane *a = node->base.plane;
> + struct drm_plane *b = new->base.plane;
> +
> + /* Komeda doesn't support setting a same zpos for
> +* different planes.
> +*/
> + DRM_DEBUG_ATOMIC("PLANE: %s and PLANE: %s are
> configured same zpos: %d.\n",
> +a->name, b->name, node->base.zpos);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_st)
> +{
> + struct drm_atomic_state *state = crtc_st->state;
> + struct komeda_plane_state *kplane_st;
> + struct drm_plane_state *plane_st;
> + struct drm_framebuffer *fb;
> + struct drm_plane *plane;
> + struct list_head zorder_list;
> + int order = 0, err;
> +
> + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
> +crtc->base.id, crtc->name);
> +
> + INIT_LIST_HEAD(_list);
> +
> + /* This loop also added all effected planes into the new state */
> + drm_for_each_plane_mask(plane, crtc->dev, crtc_st->plane_mask) {
> + plane_st =
[PATCH] drm/komeda: Adds zorder support
- Creates the zpos property.
- Implement komeda_crtc_normalize_zpos to replace
drm_atomic_normalize_zpos, reasons as the following:
1. The drm_atomic_normalize_zpos allows to configure same zpos for
different planes, but komeda doesn't support such configuration.
2. For further slave pipline case, Komeda need to calculate the
max_slave_zorder, we will merge such calculation into
komed_crtc_normalize_zpos to save a separated plane_state loop.
3. For feature none-scaling layer_split, which a plane_state will be
assigned to two individual layers(left/right), which requires two
normalize_zpos for this plane, plane_st->normalize_zpos will be used
by left layer, normalize_zpos + 1 for right_layer.
This patch series depends on:
- https://patchwork.freedesktop.org/series/58710/
- https://patchwork.freedesktop.org/series/59000/
- https://patchwork.freedesktop.org/series/59002/
- https://patchwork.freedesktop.org/series/59747/
- https://patchwork.freedesktop.org/series/59915/
- https://patchwork.freedesktop.org/series/60083/
- https://patchwork.freedesktop.org/series/60698/
Signed-off-by: Lowry Li (Arm Technology China)
---
drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 90 ++-
drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
3 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 306ea06..0ec7665 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -100,6 +100,90 @@ static void komeda_kms_commit_tail(struct drm_atomic_state
*old_state)
.atomic_commit_tail = komeda_kms_commit_tail,
};
+static int komeda_plane_state_list_add(struct drm_plane_state *plane_st,
+ struct list_head *zorder_list)
+{
+ struct komeda_plane_state *new = to_kplane_st(plane_st);
+ struct komeda_plane_state *node, *last;
+
+ last = list_empty(zorder_list) ?
+ NULL : list_last_entry(zorder_list, typeof(*last), zlist_node);
+
+ /* Considering the list sequence is zpos increasing, so if list is empty
+* or the zpos of new node bigger than the last node in list, no need
+* loop and just insert the new one to the tail of the list.
+*/
+ if (!last || (new->base.zpos > last->base.zpos)) {
+ list_add_tail(>zlist_node, zorder_list);
+ return 0;
+ }
+
+ /* Build the list by zpos increasing */
+ list_for_each_entry(node, zorder_list, zlist_node) {
+ if (new->base.zpos < node->base.zpos) {
+ list_add_tail(>zlist_node, >zlist_node);
+ break;
+ } else if (node->base.zpos == new->base.zpos) {
+ struct drm_plane *a = node->base.plane;
+ struct drm_plane *b = new->base.plane;
+
+ /* Komeda doesn't support setting a same zpos for
+* different planes.
+*/
+ DRM_DEBUG_ATOMIC("PLANE: %s and PLANE: %s are
configured same zpos: %d.\n",
+a->name, b->name, node->base.zpos);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int komeda_crtc_normalize_zpos(struct drm_crtc *crtc,
+ struct drm_crtc_state *crtc_st)
+{
+ struct drm_atomic_state *state = crtc_st->state;
+ struct komeda_plane_state *kplane_st;
+ struct drm_plane_state *plane_st;
+ struct drm_framebuffer *fb;
+ struct drm_plane *plane;
+ struct list_head zorder_list;
+ int order = 0, err;
+
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
+crtc->base.id, crtc->name);
+
+ INIT_LIST_HEAD(_list);
+
+ /* This loop also added all effected planes into the new state */
+ drm_for_each_plane_mask(plane, crtc->dev, crtc_st->plane_mask) {
+ plane_st = drm_atomic_get_plane_state(state, plane);
+ if (IS_ERR(plane_st))
+ return PTR_ERR(plane_st);
+
+ /* Build a list by zpos increasing */
+ err = komeda_plane_state_list_add(plane_st, _list);
+ if (err)
+ return err;
+ }
+
+ list_for_each_entry(kplane_st, _list, zlist_node) {
+ plane_st = _st->base;
+ fb = plane_st->fb;
+ plane = plane_st->plane;
+
+ plane_st->normalized_zpos = order++;
+
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] zpos:%d, normalized zpos: %d\n",
+plane->base.id, plane->name,
+plane_st->zpos, plane_st->normalized_zpos);
+ }
+
+
