Re: [PATCH] drm/komeda: Adds zorder support

From: james qian wang (Arm Technology China)
Date: Wed Oct 09 2019 - 01:15:57 EST

Next message: Roman Gushchin: "Re: [PATCH] cgroup, blkcg: prevent dirty inodes to pin dying memory cgroups"
Previous message: Stephen Rothwell: "linux-next: Tree for Oct 9"
In reply to: Daniel Vetter: "Re: [PATCH] drm/komeda: Adds zorder support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Next message: Roman Gushchin: "Re: [PATCH] cgroup, blkcg: prevent dirty inodes to pin dying memory cgroups"
Previous message: Stephen Rothwell: "linux-next: Tree for Oct 9"
In reply to: Daniel Vetter: "Re: [PATCH] drm/komeda: Adds zorder support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]