Hi Maxime, for the whole series: Acked-by: Thomas Zimmermann Am 19.02.21 um 13:00 schrieb Maxime Ripard: > The current atomic helpers have either their object state being passed as > an argument or the full atomic state. > > The former is the pattern that was done at first, before switching to the > latter for new hooks or when it was needed. > > Let's start convert all the remaining helpers to provide a consistent > interface, starting with the planes atomic_async_check and > atomic_async_update. > > The conversion was done using the coccinelle script below, built tested on > all the drivers. > > @@ > identifier plane, plane_state; > symbol state; > @@ > > struct drm_plane_helper_funcs { > ... > int (*atomic_async_check)(struct drm_plane *plane, > - struct drm_plane_state *plane_state); > + struct drm_atomic_state *state); > ... > } > > @@ > identifier plane, plane_state; > symbol state; > @@ > struct drm_plane_helper_funcs { > ... > void (*atomic_async_update)(struct drm_plane *plane, > - struct drm_plane_state *plane_state); > + struct drm_atomic_state *state); > ... > } > > @ plane_atomic_func @ > identifier helpers; > identifier func; > @@ > > ( > static const struct drm_plane_helper_funcs helpers = { > ..., > .atomic_async_check = func, > ..., > }; > | > static const struct drm_plane_helper_funcs helpers = { > ..., > .atomic_async_update = func, > ..., > }; > ) > > @@ > struct drm_plane_helper_funcs *FUNCS; > identifier f; > identifier dev; > identifier plane, plane_state, state; > @@ > > f(struct drm_device *dev, struct drm_atomic_state *state) > { > <+... > - FUNCS->atomic_async_check(plane, plane_state) > + FUNCS->atomic_async_check(plane, state) > ...+> > } > > @@ > struct drm_plane_helper_funcs *FUNCS; > identifier f; > identifier dev; > identifier plane, plane_state, state; > @@ > > f(struct drm_device *dev, struct drm_atomic_state *state) > { > <+... > - FUNCS->atomic_async_update(plane, plane_state) > + FUNCS->atomic_async_update(plane, state) > ...+> > } > > @@ > identifier mtk_plane_atomic_async_update; > identifier plane; > symbol new_state, state; > expression e; > @@ > > void mtk_plane_atomic_async_update(struct drm_plane *plane, struct drm_plane_state *new_state) > { > ... > - struct mtk_plane_state *state = e; > + struct mtk_plane_state *new_plane_state = e; > <+... > - state > + new_plane_state > ...+> > } > > @@ > identifier plane_atomic_func.func; > identifier plane; > symbol state; > @@ > > func(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_plane_state *new_plane_state) > { > <... > - state > + new_plane_state > ...> > } > > @ ignores_new_state @ > identifier plane_atomic_func.func; > identifier plane, new_plane_state; > @@ > > func(struct drm_plane *plane, struct drm_plane_state *new_plane_state) > { > ... when != new_plane_state > } > > @ adds_new_state depends on plane_atomic_func && !ignores_new_state @ > identifier plane_atomic_func.func; > identifier plane, new_plane_state; > @@ > > func(struct drm_plane *plane, struct drm_plane_state *new_plane_state) > { > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); > ... > } > > @ depends on plane_atomic_func @ > identifier plane_atomic_func.func; > identifier plane, plane_state; > @@ > > func(struct drm_plane *plane, > - struct drm_plane_state *plane_state > + struct drm_atomic_state *state > ) > { ... } > > @ include depends on adds_new_state @ > @@ > > #include > > @ no_include depends on !include && adds_new_state @ > @@ > > + #include > #include > > @@ > identifier plane_atomic_func.func; > identifier plane, state; > identifier plane_state; > @@ > > func(struct drm_plane *plane, struct drm_atomic_state *state) { > ... > struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); > <+... > - plane_state->state > + state > ...+> > } > > Acked-by: Thomas Zimmermann > Signed-off-by: Maxime Ripard > > --- > > Changes from v1: > - Updated the comment according to Thomas suggestions > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++- > drivers/gpu/drm/drm_atomic_helper.c | 4 +- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 26 +++++---- > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 33 ++++++----- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 16 ++++-- > drivers/gpu/drm/vc4/vc4_plane.c | 56 ++++++++++--------- > include/drm/drm_modeset_helper_vtables.h | 18 +++--- > 7 files changed, 89 insertions(+), 72 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 6ed96633425f..63f839679a0a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6468,7 +6468,7 @@ static int dm_plane_atomic_check(struct drm_plane *plane, > } > > static int dm_plane_atomic_async_check(struct drm_plane *plane, > - struct drm_plane_state *new_plane_state) > + struct drm_atomic_state *state) > { > /* Only support async updates on cursor planes. */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) > @@ -6478,10 +6478,12 @@ static int dm_plane_atomic_async_check(struct drm_plane *plane, > } > > static void dm_plane_atomic_async_update(struct drm_plane *plane, > - struct drm_plane_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, > + plane); > struct drm_plane_state *old_state = > - drm_atomic_get_old_plane_state(new_state->state, plane); > + drm_atomic_get_old_plane_state(state, plane); > > trace_amdgpu_dm_atomic_update_cursor(new_state); > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 9fa3f97223a1..bd0bb74437c7 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1742,7 +1742,7 @@ int drm_atomic_helper_async_check(struct drm_device *dev, > return -EBUSY; > } > > - return funcs->atomic_async_check(plane, new_plane_state); > + return funcs->atomic_async_check(plane, state); > } > EXPORT_SYMBOL(drm_atomic_helper_async_check); > > @@ -1772,7 +1772,7 @@ void drm_atomic_helper_async_commit(struct drm_device *dev, > struct drm_framebuffer *old_fb = plane->state->fb; > > funcs = plane->helper_private; > - funcs->atomic_async_update(plane, plane_state); > + funcs->atomic_async_update(plane, state); > > /* > * ->atomic_async_update() is supposed to update the > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 92141a19681b..30ebcfd8832f 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -77,12 +77,14 @@ static void mtk_drm_plane_destroy_state(struct drm_plane *plane, > } > > static int mtk_plane_atomic_async_check(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > struct drm_crtc_state *crtc_state; > int ret; > > - if (plane != state->crtc->cursor) > + if (plane != new_plane_state->crtc->cursor) > return -EINVAL; > > if (!plane->state) > @@ -91,16 +93,16 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane, > if (!plane->state->fb) > return -EINVAL; > > - ret = mtk_drm_crtc_plane_check(state->crtc, plane, > - to_mtk_plane_state(state)); > + ret = mtk_drm_crtc_plane_check(new_plane_state->crtc, plane, > + to_mtk_plane_state(new_plane_state)); > if (ret) > return ret; > > - if (state->state) > - crtc_state = drm_atomic_get_existing_crtc_state(state->state, > - state->crtc); > + if (state) > + crtc_state = drm_atomic_get_existing_crtc_state(state, > + new_plane_state->crtc); > else /* Special case for asynchronous cursor updates. */ > - crtc_state = state->crtc->state; > + crtc_state = new_plane_state->crtc->state; > > return drm_atomic_helper_check_plane_state(plane->state, crtc_state, > DRM_PLANE_HELPER_NO_SCALING, > @@ -109,9 +111,11 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane, > } > > static void mtk_plane_atomic_async_update(struct drm_plane *plane, > - struct drm_plane_state *new_state) > + struct drm_atomic_state *state) > { > - struct mtk_plane_state *state = to_mtk_plane_state(plane->state); > + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, > + plane); > + struct mtk_plane_state *new_plane_state = to_mtk_plane_state(plane->state); > > plane->state->crtc_x = new_state->crtc_x; > plane->state->crtc_y = new_state->crtc_y; > @@ -122,7 +126,7 @@ static void mtk_plane_atomic_async_update(struct drm_plane *plane, > plane->state->src_h = new_state->src_h; > plane->state->src_w = new_state->src_w; > swap(plane->state->fb, new_state->fb); > - state->pending.async_dirty = true; > + new_plane_state->pending.async_dirty = true; > > mtk_drm_crtc_async_update(new_state->crtc, plane, new_state); > } > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c > index 83423092de2f..05fa8255caeb 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c > @@ -5,6 +5,7 @@ > * Author: Rob Clark > */ > > +#include > #include > #include > #include > @@ -438,41 +439,43 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, > } > > static int mdp5_plane_atomic_async_check(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_atomic_state *state) > { > - struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > + struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(new_plane_state); > struct drm_crtc_state *crtc_state; > int min_scale, max_scale; > int ret; > > - crtc_state = drm_atomic_get_existing_crtc_state(state->state, > - state->crtc); > + crtc_state = drm_atomic_get_existing_crtc_state(state, > + new_plane_state->crtc); > if (WARN_ON(!crtc_state)) > return -EINVAL; > > if (!crtc_state->active) > return -EINVAL; > > - mdp5_state = to_mdp5_plane_state(state); > + mdp5_state = to_mdp5_plane_state(new_plane_state); > > /* don't use fast path if we don't have a hwpipe allocated yet */ > if (!mdp5_state->hwpipe) > return -EINVAL; > > /* only allow changing of position(crtc x/y or src x/y) in fast path */ > - if (plane->state->crtc != state->crtc || > - plane->state->src_w != state->src_w || > - plane->state->src_h != state->src_h || > - plane->state->crtc_w != state->crtc_w || > - plane->state->crtc_h != state->crtc_h || > + if (plane->state->crtc != new_plane_state->crtc || > + plane->state->src_w != new_plane_state->src_w || > + plane->state->src_h != new_plane_state->src_h || > + plane->state->crtc_w != new_plane_state->crtc_w || > + plane->state->crtc_h != new_plane_state->crtc_h || > !plane->state->fb || > - plane->state->fb != state->fb) > + plane->state->fb != new_plane_state->fb) > return -EINVAL; > > min_scale = FRAC_16_16(1, 8); > max_scale = FRAC_16_16(8, 1); > > - ret = drm_atomic_helper_check_plane_state(state, crtc_state, > + ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > min_scale, max_scale, > true, true); > if (ret) > @@ -485,15 +488,17 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane, > * also assign/unassign the hwpipe(s) tied to the plane. We avoid > * taking the fast path for both these reasons. > */ > - if (state->visible != plane->state->visible) > + if (new_plane_state->visible != plane->state->visible) > return -EINVAL; > > return 0; > } > > static void mdp5_plane_atomic_async_update(struct drm_plane *plane, > - struct drm_plane_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, > + plane); > struct drm_framebuffer *old_fb = plane->state->fb; > > plane->state->src_x = new_state->src_x; > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index 8d15cabdcb02..fefeab73ca27 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -1021,8 +1021,10 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > } > > static int vop_plane_atomic_async_check(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > struct vop_win *vop_win = to_vop_win(plane); > const struct vop_win_data *win = vop_win->data; > int min_scale = win->phy->scl ? FRAC_16_16(1, 8) : > @@ -1031,7 +1033,7 @@ static int vop_plane_atomic_async_check(struct drm_plane *plane, > DRM_PLANE_HELPER_NO_SCALING; > struct drm_crtc_state *crtc_state; > > - if (plane != state->crtc->cursor) > + if (plane != new_plane_state->crtc->cursor) > return -EINVAL; > > if (!plane->state) > @@ -1040,9 +1042,9 @@ static int vop_plane_atomic_async_check(struct drm_plane *plane, > if (!plane->state->fb) > return -EINVAL; > > - if (state->state) > - crtc_state = drm_atomic_get_existing_crtc_state(state->state, > - state->crtc); > + if (state) > + crtc_state = drm_atomic_get_existing_crtc_state(state, > + new_plane_state->crtc); > else /* Special case for asynchronous cursor updates. */ > crtc_state = plane->crtc->state; > > @@ -1052,8 +1054,10 @@ static int vop_plane_atomic_async_check(struct drm_plane *plane, > } > > static void vop_plane_atomic_async_update(struct drm_plane *plane, > - struct drm_plane_state *new_state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, > + plane); > struct vop *vop = to_vop(plane->state->crtc); > struct drm_framebuffer *old_fb = plane->state->fb; > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > index 6bd8260aa9f2..ff485e58d271 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -1118,32 +1118,34 @@ void vc4_plane_async_set_fb(struct drm_plane *plane, struct drm_framebuffer *fb) > } > > static void vc4_plane_atomic_async_update(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > struct vc4_plane_state *vc4_state, *new_vc4_state; > > - swap(plane->state->fb, state->fb); > - plane->state->crtc_x = state->crtc_x; > - plane->state->crtc_y = state->crtc_y; > - plane->state->crtc_w = state->crtc_w; > - plane->state->crtc_h = state->crtc_h; > - plane->state->src_x = state->src_x; > - plane->state->src_y = state->src_y; > - plane->state->src_w = state->src_w; > - plane->state->src_h = state->src_h; > - plane->state->src_h = state->src_h; > - plane->state->alpha = state->alpha; > - plane->state->pixel_blend_mode = state->pixel_blend_mode; > - plane->state->rotation = state->rotation; > - plane->state->zpos = state->zpos; > - plane->state->normalized_zpos = state->normalized_zpos; > - plane->state->color_encoding = state->color_encoding; > - plane->state->color_range = state->color_range; > - plane->state->src = state->src; > - plane->state->dst = state->dst; > - plane->state->visible = state->visible; > + swap(plane->state->fb, new_plane_state->fb); > + plane->state->crtc_x = new_plane_state->crtc_x; > + plane->state->crtc_y = new_plane_state->crtc_y; > + plane->state->crtc_w = new_plane_state->crtc_w; > + plane->state->crtc_h = new_plane_state->crtc_h; > + plane->state->src_x = new_plane_state->src_x; > + plane->state->src_y = new_plane_state->src_y; > + plane->state->src_w = new_plane_state->src_w; > + plane->state->src_h = new_plane_state->src_h; > + plane->state->src_h = new_plane_state->src_h; > + plane->state->alpha = new_plane_state->alpha; > + plane->state->pixel_blend_mode = new_plane_state->pixel_blend_mode; > + plane->state->rotation = new_plane_state->rotation; > + plane->state->zpos = new_plane_state->zpos; > + plane->state->normalized_zpos = new_plane_state->normalized_zpos; > + plane->state->color_encoding = new_plane_state->color_encoding; > + plane->state->color_range = new_plane_state->color_range; > + plane->state->src = new_plane_state->src; > + plane->state->dst = new_plane_state->dst; > + plane->state->visible = new_plane_state->visible; > > - new_vc4_state = to_vc4_plane_state(state); > + new_vc4_state = to_vc4_plane_state(new_plane_state); > vc4_state = to_vc4_plane_state(plane->state); > > vc4_state->crtc_x = new_vc4_state->crtc_x; > @@ -1187,23 +1189,25 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane, > } > > static int vc4_plane_atomic_async_check(struct drm_plane *plane, > - struct drm_plane_state *state) > + struct drm_atomic_state *state) > { > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > struct vc4_plane_state *old_vc4_state, *new_vc4_state; > int ret; > u32 i; > > - ret = vc4_plane_mode_set(plane, state); > + ret = vc4_plane_mode_set(plane, new_plane_state); > if (ret) > return ret; > > old_vc4_state = to_vc4_plane_state(plane->state); > - new_vc4_state = to_vc4_plane_state(state); > + new_vc4_state = to_vc4_plane_state(new_plane_state); > if (old_vc4_state->dlist_count != new_vc4_state->dlist_count || > old_vc4_state->pos0_offset != new_vc4_state->pos0_offset || > old_vc4_state->pos2_offset != new_vc4_state->pos2_offset || > old_vc4_state->ptr0_offset != new_vc4_state->ptr0_offset || > - vc4_lbm_size(plane->state) != vc4_lbm_size(state)) > + vc4_lbm_size(plane->state) != vc4_lbm_size(new_plane_state)) > return -EINVAL; > > /* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index eb706342861d..2b4d085da450 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -1292,9 +1292,9 @@ struct drm_plane_helper_funcs { > /** > * @atomic_async_check: > * > - * Drivers should set this function pointer to check if the plane state > - * can be updated in a async fashion. Here async means "not vblank > - * synchronized". > + * Drivers should set this function pointer to check if the plane's > + * atomic state can be updated in a async fashion. Here async means > + * "not vblank synchronized". > * > * This hook is called by drm_atomic_async_check() to establish if a > * given update can be committed asynchronously, that is, if it can > @@ -1306,7 +1306,7 @@ struct drm_plane_helper_funcs { > * can not be applied in asynchronous manner. > */ > int (*atomic_async_check)(struct drm_plane *plane, > - struct drm_plane_state *state); > + struct drm_atomic_state *state); > > /** > * @atomic_async_update: > @@ -1322,11 +1322,9 @@ struct drm_plane_helper_funcs { > * update won't happen if there is an outstanding commit modifying > * the same plane. > * > - * Note that unlike &drm_plane_helper_funcs.atomic_update this hook > - * takes the new &drm_plane_state as parameter. When doing async_update > - * drivers shouldn't replace the &drm_plane_state but update the > - * current one with the new plane configurations in the new > - * plane_state. > + * When doing async_update drivers shouldn't replace the > + * &drm_plane_state but update the current one with the new plane > + * configurations in the new plane_state. > * > * Drivers should also swap the framebuffers between current plane > * state (&drm_plane.state) and new_state. > @@ -1345,7 +1343,7 @@ struct drm_plane_helper_funcs { > * for deferring if needed, until a common solution is created. > */ > void (*atomic_async_update)(struct drm_plane *plane, > - struct drm_plane_state *new_state); > + struct drm_atomic_state *state); > }; > > /** > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer