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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
