From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 12/19] drm/msm: Use new atomic iterator macros
Date: Thu, 3 Nov 2016 18:37:06 +0200 [thread overview]
Message-ID: <20161103163706.GC4617@intel.com> (raw)
In-Reply-To: <1476707838-25253-13-git-send-email-maarten.lankhorst@linux.intel.com>
On Mon, Oct 17, 2016 at 02:37:11PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 4 ++--
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 6 +++---
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 3 ++-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++--
> drivers/gpu/drm/msm/msm_atomic.c | 16 ++++++++--------
> 5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> index 571a91ee9607..d18d0a0e0a35 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> @@ -113,7 +113,7 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
> mdp4_enable(mdp4_kms);
>
> /* see 119ecb7fd */
> - for_each_crtc_in_state(state, crtc, crtc_state, i)
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> drm_crtc_vblank_get(crtc);
> }
>
> @@ -125,7 +125,7 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
> struct drm_crtc_state *crtc_state;
>
> /* see 119ecb7fd */
> - for_each_crtc_in_state(state, crtc, crtc_state, i)
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> drm_crtc_vblank_put(crtc);
post-swap, but crtc_state not used in either. So lgtm.
I wonder if you should go all in and try to rename all the foo_state variables
as new_foo_state and old_foo_state to make sure you didn't miss any uses which
might not agree with your new choice if iterator macro? Eg. if in this
case crtc_state was actually used but you failed notice it you would have
just broken something by using the _new_ iterator.
>
> mdp4_disable(mdp4_kms);
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index ed7143d35b25..7cfeb0455039 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -82,10 +82,10 @@ static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
> int i;
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> struct drm_plane *plane;
> - struct drm_plane_state *plane_state;
> + struct drm_plane_state *old_state, *new_state;
>
> - for_each_plane_in_state(state, plane, plane_state, i)
> - mdp5_plane_complete_commit(plane, plane_state);
> + for_each_oldnew_plane_in_state(state, plane, old_state, new_state, i)
> + mdp5_plane_complete_commit(plane, old_state, new_state);
>
> mdp5_disable(mdp5_kms);
> }
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index 03738927be10..90e80619fc54 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -198,7 +198,8 @@ void mdp5_irq_domain_fini(struct mdp5_kms *mdp5_kms);
> uint32_t mdp5_plane_get_flush(struct drm_plane *plane);
> void mdp5_plane_complete_flip(struct drm_plane *plane);
> void mdp5_plane_complete_commit(struct drm_plane *plane,
> - struct drm_plane_state *state);
> + struct drm_plane_state *old_state,
> + struct drm_plane_state *new_state);
> enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane);
> struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> enum mdp5_pipe pipe, bool private_plane,
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 951c002b05df..19c44b968f4e 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -859,13 +859,13 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane)
>
> /* called after vsync in thread context */
> void mdp5_plane_complete_commit(struct drm_plane *plane,
> - struct drm_plane_state *state)
> + struct drm_plane_state *old_state, struct drm_plane_state *new_state)
> {
> struct mdp5_kms *mdp5_kms = get_kms(plane);
> struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
> enum mdp5_pipe pipe = mdp5_plane->pipe;
>
> - if (!plane_enabled(plane->state) && mdp5_kms->smp) {
> + if (!plane_enabled(new_state) && mdp5_kms->smp) {
> DBG("%s: free SMP", mdp5_plane->name);
> mdp5_smp_release(mdp5_kms->smp, pipe);
> }
Doesn't look like this guy actually needs the old state. Not sure why it
was getting passed in. The replacement for plane->state looks correct
nonetheless.
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index db193f835298..333c379e6561 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -89,8 +89,8 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev,
> struct msm_kms *kms = priv->kms;
> int i;
>
> - for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> - if (!crtc->state->enable)
> + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
> + if (!crtc_state->enable)
> continue;
post-swap, lgtm.
>
> /* Legacy cursor ioctls are completely unsynced, and userspace
> @@ -191,7 +191,7 @@ int msm_atomic_commit(struct drm_device *dev,
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> struct drm_plane *plane;
> - struct drm_plane_state *plane_state;
> + struct drm_plane_state *new_plane_state, *old_plane_state;
> int i, ret;
>
> ret = drm_atomic_helper_prepare_planes(dev, state);
> @@ -207,18 +207,18 @@ int msm_atomic_commit(struct drm_device *dev,
> /*
> * Figure out what crtcs we have:
> */
> - for_each_crtc_in_state(state, crtc, crtc_state, i)
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> c->crtc_mask |= drm_crtc_mask(crtc);
>
> /*
> * Figure out what fence to wait for:
> */
> - for_each_plane_in_state(state, plane, plane_state, i) {
> - if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
> - struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> + if (old_plane_state->fb != new_plane_state->fb && new_plane_state->fb) {
> + struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0);
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> - plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
> + new_plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
> }
pre-swap, lgtm.
Patches 08-12
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> }
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-11-03 16:37 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-17 12:36 [PATCH 00/19] drm/atomic: Use less confusing iterator names Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 01/19] drm/atomic: Add new iterators over all state Maarten Lankhorst
2016-11-01 13:09 ` [Intel-gfx] " Ville Syrjälä
2016-11-01 13:34 ` Maarten Lankhorst
2016-11-01 13:41 ` Ville Syrjälä
2016-11-02 8:28 ` Maarten Lankhorst
2016-11-03 15:11 ` [Intel-gfx] " Ville Syrjälä
2016-11-16 12:56 ` Maarten Lankhorst
2016-11-08 8:50 ` [Intel-gfx] " Daniel Vetter
2016-10-17 12:37 ` [PATCH 02/19] drm/atmel-hlcdc: Use new atomic iterator macros Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 03/19] drm/exynos: " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 04/19] drm/blend: " Maarten Lankhorst
2016-11-03 15:26 ` Ville Syrjälä
2016-10-17 12:37 ` [PATCH 05/19] drm/atomic: Make add_affected_connectors look at crtc_state Maarten Lankhorst
2016-11-03 15:32 ` Ville Syrjälä
2016-11-08 8:51 ` [Intel-gfx] " Daniel Vetter
2016-10-17 12:37 ` [PATCH 06/19] drm/atomic: Use new atomic iterator macros Maarten Lankhorst
2016-11-03 15:35 ` [Intel-gfx] " Ville Syrjälä
2016-10-17 12:37 ` [PATCH 07/19] drm/atomic: Fix atomic helpers to use the new " Maarten Lankhorst
2016-11-03 16:22 ` Ville Syrjälä
2016-11-08 8:53 ` Daniel Vetter
2016-10-17 12:37 ` [PATCH 08/19] drm/vc4: Use new atomic " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 09/19] drm/rockchip: " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 10/19] drm/rcar-du: " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 11/19] drm/omap: " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 12/19] drm/msm: " Maarten Lankhorst
2016-11-03 16:37 ` Ville Syrjälä [this message]
2016-10-17 12:37 ` [PATCH 13/19] drm/imx: " Maarten Lankhorst
2016-10-17 12:37 ` [PATCH 14/19] drm/mediatek: " Maarten Lankhorst
2016-11-03 16:39 ` Ville Syrjälä
2016-10-17 12:37 ` [PATCH 15/19] drm/i915: Use new atomic iterator macros in ddi Maarten Lankhorst
2016-11-03 17:07 ` Ville Syrjälä
2016-10-17 12:37 ` [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc Maarten Lankhorst
2016-11-03 16:45 ` Ville Syrjälä
2016-11-03 17:45 ` Paulo Zanoni
2016-11-03 17:55 ` [Intel-gfx] " Ville Syrjälä
2016-11-03 17:59 ` Ville Syrjälä
2016-10-17 12:37 ` [PATCH 17/19] drm/i915: Use new atomic iterator macros in wm code Maarten Lankhorst
2016-11-03 16:49 ` [Intel-gfx] " Ville Syrjälä
2016-11-03 17:53 ` Paulo Zanoni
2016-10-17 12:37 ` [PATCH 18/19] drm/i915: Use new atomic iterator macros in display code Maarten Lankhorst
2016-11-03 17:04 ` [Intel-gfx] " Ville Syrjälä
2016-10-17 12:37 ` [PATCH 19/19] drm/atomic: Rename atomic oldnew iterator Maarten Lankhorst
2016-11-03 17:11 ` [Intel-gfx] " Ville Syrjälä
2016-10-17 13:25 ` ✗ Fi.CI.BAT: warning for drm/atomic: Use less confusing iterator names Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161103163706.GC4617@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.