On Wed, Oct 28, 2020 at 01:32:21PM +0100, Maxime Ripard wrote: > 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 CRTC's atomic_check. > > The conversion was done using the coccinelle script below, > built tested on all the drivers and actually tested on vc4. > > virtual report ? <snip> > @ depends on crtc_atomic_func @ > identifier crtc_atomic_func.func; > expression E; > type T; > @@ > > int func(...) > { > ... > - T state = E; > + T crtc_state = E; > <+... > - state > + crtc_state > ...+> > } > > @ depends on crtc_atomic_func @ > identifier crtc_atomic_func.func; > type T; > @@ > > int func(...) > { > ... > - T state; > + T crtc_state; > <+... > - state > + crtc_state > ...+> > } These two seem a bit fuzzy. AFAICS 'state' could be any kind of state given the constrainsts. Though I guess the fact that this is the crtc .atomic_check() it's most likely to either the crtc state or the whole atomic state. Not sure what would be the best way to tighten this up. Maybe a regex thing on the assignment? But I'm not sure you can even do that on an expression. Anyways, doesn't look like this went wrong anywhere, so seems good enough for a onetime job. <snip> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index 956f631997f2..b0757f84a979 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -269,17 +269,19 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > } > > static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc, > - struct drm_crtc_state *state) > + struct drm_atomic_state *state) > { > - bool has_primary = state->plane_mask & > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > + crtc); > + bool has_primary = crtc_state->plane_mask & > drm_plane_mask(crtc->primary); > > /* The primary plane has to be enabled when the CRTC is active. */ > - if (state->active && !has_primary) > + if (crtc_state->active && !has_primary) > return -EINVAL; > > /* TODO: Is this needed ? */ > - return drm_atomic_add_affected_planes(state->state, crtc); > + return drm_atomic_add_affected_planes(crtc_state->state, crtc); Could also s/crtc_state->state/state/ in various places. But that could done as a followup as well. Didn't spot any mistakes: Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel