From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f194.google.com ([209.85.220.194]:35900 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752220AbcJJPbi (ORCPT ); Mon, 10 Oct 2016 11:31:38 -0400 Received: by mail-qk0-f194.google.com with SMTP id z190so7124372qkc.3 for ; Mon, 10 Oct 2016 08:31:37 -0700 (PDT) Date: Mon, 10 Oct 2016 17:31:34 +0200 From: Daniel Vetter To: ville.syrjala@linux.intel.com Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Marek Szyprowski , Benjamin Gaignard , Vincent Abriou , Laurent Pinchart , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Lyude , Maarten Lankhorst , stable@vger.kernel.org Subject: Re: [PATCH v2] drm: Don't force all planes to be added to the state due to zpos Message-ID: <20161010153134.GX20761@phenom.ffwll.local> References: <1476101987-31986-1-git-send-email-ville.syrjala@linux.intel.com> <1476111056-12734-1-git-send-email-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1476111056-12734-1-git-send-email-ville.syrjala@linux.intel.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Oct 10, 2016 at 05:50:56PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrj�l� > > We don't want all planes to be added to the state whenever a > plane with fixed zpos gets enabled/disabled. This is true > especially for eg. cursor planes on i915, as we want cursor > updates to go through w/o throttling. Same holds for drivers > that don't support zpos at all (i915 actually falls into this > category right now since we've not yet added zpos support). > > Allow drivers more freedom by letting them deal with zpos > themselves instead of doing it in drm_atomic_helper_check_planes() > unconditionally. Let's just inline the required calls into all > the driver that currently depend on this. > > v2: Inline the stuff into the drivers instead of adding another > helper, document things better (Daniel) > > Cc: Daniel Vetter > Cc: Marek Szyprowski > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Laurent Pinchart > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Lyude > Cc: Maarten Lankhorst > Cc: stable@vger.kernel.org > Fixes: 44d1240d006c ("drm: add generic zpos property") > Signed-off-by: Ville Syrj�l� > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ---- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + > drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 12 ++++++++++-- > drivers/gpu/drm/sti/sti_drv.c | 22 +++++++++++++++++++++- > include/drm/drm_plane.h | 8 +++++++- > 7 files changed, 60 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c3f83476f996..21f992605541 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -594,10 +594,6 @@ drm_atomic_helper_check_planes(struct drm_device *dev, > struct drm_plane_state *plane_state; > int i, ret = 0; > > - ret = drm_atomic_normalize_zpos(dev, state); > - if (ret) > - return ret; > - > for_each_plane_in_state(state, plane, plane_state, i) { > const struct drm_plane_helper_funcs *funcs; > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index def78c8c1780..f86e7c846678 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -262,6 +262,26 @@ int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, > return 0; > } > > +int exynos_atomic_check(struct drm_device *dev, > + struct drm_atomic_state *state) > +{ > + int ret; > + > + ret = drm_atomic_helper_check_modeset(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_normalize_zpos(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_helper_check_planes(dev, state); > + if (ret) > + return ret; > + > + return ret; > +} > + > static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) > { > struct drm_exynos_file_private *file_priv; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index d215149e737b..80c4d5b81689 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -301,6 +301,7 @@ static inline int exynos_dpi_bind(struct drm_device *dev, > > int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, > bool nonblock); > +int exynos_atomic_check(struct drm_device *dev, struct drm_atomic_state *state); > > > extern struct platform_driver fimd_driver; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c > index 40ce841eb952..23cce0a3f5fc 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c > @@ -190,7 +190,7 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index) > static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { > .fb_create = exynos_user_fb_create, > .output_poll_changed = exynos_drm_output_poll_changed, > - .atomic_check = drm_atomic_helper_check, > + .atomic_check = exynos_atomic_check, > .atomic_commit = exynos_atomic_commit, > }; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > index bd9c3bb9252c..392c7e6de042 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -231,8 +231,16 @@ static int rcar_du_atomic_check(struct drm_device *dev, > struct rcar_du_device *rcdu = dev->dev_private; > int ret; > > - ret = drm_atomic_helper_check(dev, state); > - if (ret < 0) > + ret = drm_atomic_helper_check_modeset(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_normalize_zpos(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_helper_check_planes(dev, state); > + if (ret) > return ret; > > if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) > diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c > index 2784919a7366..9df308565f6c 100644 > --- a/drivers/gpu/drm/sti/sti_drv.c > +++ b/drivers/gpu/drm/sti/sti_drv.c > @@ -195,6 +195,26 @@ static void sti_atomic_work(struct work_struct *work) > sti_atomic_complete(private, private->commit.state); > } > > +static int sti_atomic_check(struct drm_device *dev, > + struct drm_atomic_state *state) > +{ > + int ret; > + > + ret = drm_atomic_helper_check_modeset(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_normalize_zpos(dev, state); > + if (ret) > + return ret; > + > + ret = drm_atomic_helper_check_planes(dev, state); > + if (ret) > + return ret; > + > + return ret; > +} > + > static int sti_atomic_commit(struct drm_device *drm, > struct drm_atomic_state *state, bool nonblock) > { > @@ -248,7 +268,7 @@ static void sti_output_poll_changed(struct drm_device *ddev) > static const struct drm_mode_config_funcs sti_mode_config_funcs = { > .fb_create = drm_fb_cma_create, > .output_poll_changed = sti_output_poll_changed, > - .atomic_check = drm_atomic_helper_check, > + .atomic_check = sti_atomic_check, > .atomic_commit = sti_atomic_commit, > }; > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 43cf193e54d6..8b4dc62470ff 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -47,8 +47,14 @@ struct drm_crtc; > * @src_h: height of visible portion of plane (in 16.16) > * @rotation: rotation of the plane > * @zpos: priority of the given plane on crtc (optional) > + * Note that multiple active planes on the same crtc can have an identical > + * zpos value. The rule to solving the conflict is to compare the plane > + * object IDs; the plane with a higher ID must be stacked on top of a > + * plane with a lower ID. > * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1 > - * where N is the number of active planes for given crtc > + * where N is the number of active planes for given crtc. Note that > + * the driver must call drm_atomic_normalize_zpos() to update this before > + * it can be trusted. Hm, for longer kerneldoc for struct members I prefer the in-line layout. More readable overall, and better grouped. Either way: Reviewed-by: Daniel Vetter I'll pick it up once driver maintainers had a bit of time to ack it. -Daniel > * @src: clipped source coordinates of the plane (in 16.16) > * @dst: clipped destination coordinates of the plane > * @visible: visibility of the plane > -- > 2.7.4 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch