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) Lowry.Li@arm.com 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) lowry.li@arm.com

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(&new->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(&new->zlist_node, &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(&zorder_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, &zorder_list);
  

•           if (err)
  

•                   return err;
  

•   }
  

•   list_for_each_entry(kplane_st, &zorder_list, zlist_node) {
  

•           plane_st = &kplane_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);
  

•   }
  

•   crtc_st->zpos_changed = true;
  

•   return 0;
  

+}

static int komeda_kms_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -111,7 +195,7 @@ static int komeda_kms_check(struct drm_device *dev, if (err) return err;

•   /* komeda need to re-calculate resource assumption in every commit
  

•   /* Komeda need to re-calculate resource assumption in every commit
     * so need to add all affected_planes (even unchanged) to
     * drm_atomic_state.
     */
  

@@ -119,6 +203,10 @@ static int komeda_kms_check(struct drm_device *dev, err = drm_atomic_add_affected_planes(state, crtc); if (err) return err;

•           err = komeda_crtc_normalize_zpos(crtc, new_crtc_st);
  

•           if (err)
  

•                   return err;
    }
  
    err = drm_atomic_helper_check_planes(dev, state);
  

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h index 178bee6..d1cef46 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h @@ -7,6 +7,7 @@ #ifndef _KOMEDA_KMS_H_ #define _KOMEDA_KMS_H_

+#include <linux/list.h> #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> @@ -46,6 +47,8 @@ struct komeda_plane { struct komeda_plane_state { /** @base: &drm_plane_state */ struct drm_plane_state base;

•   /** @zlist_node: zorder list node */
  

•   struct list_head zlist_node;
  
    /* @img_enhancement: on/off image enhancement */
    u8 img_enhancement : 1;
  

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c index bcf30a7..aad7663 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c @@ -21,7 +21,7 @@

    memset(dflow, 0, sizeof(*dflow));

•   dflow->blending_zorder = st->zpos;
  

•   dflow->blending_zorder = st->normalized_zpos;
  
    /* if format doesn't have alpha, fix blend mode to PIXEL_NONE */
    dflow->pixel_blend_mode = fb->format->has_alpha ?
  

@@ -343,6 +343,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms, if (err) goto cleanup;

•   err = drm_plane_create_zpos_property(plane, layer->base.id, 0, 8);
  

•   if (err)
  

•           goto cleanup;
  

•   return 0;
  

cleanup: komeda_plane_destroy(plane); -- 1.9.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch