Hi Ville, On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote: > On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > > Many drivers reference the crtc->pointer in order to get the current CRTC > > state in their atomic_begin or atomic_flush hooks, which would be the new > > CRTC state in the global atomic state since _swap_state happened when those > > hooks are run. > > > > Use the drm_atomic_get_new_crtc_state helper to get that state to make it > > more obvious. > > > > This was made using the coccinelle script below: > > > > @ crtc_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > > > > ( > > static struct drm_crtc_helper_funcs helpers = { > > ..., > > .atomic_begin = func, > > ..., > > }; > > | > > static struct drm_crtc_helper_funcs helpers = { > > ..., > > .atomic_flush = func, > > ..., > > }; > > ) > > > > @@ > > identifier crtc_atomic_func.func; > > identifier crtc, state; > > symbol crtc_state; > > expression e; > > @@ > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > ... > > - struct tegra_dc_state *crtc_state = e; > > + struct tegra_dc_state *dc_state = e; > > <+... > > - crtc_state > > + dc_state > > ...+> > > } > > > > @@ > > identifier crtc_atomic_func.func; > > identifier crtc, state; > > symbol crtc_state; > > expression e; > > @@ > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > ... > > - struct mtk_crtc_state *crtc_state = e; > > + struct mtk_crtc_state *mtk_crtc_state = e; > > <+... > > - crtc_state > > + mtk_crtc_state > > ...+> > > } > > These reanames seem a bit out of scpe for this patch. But I guess you > needed then to get the rest of the cocci to work on some drivers? Yeah, those two drivers already had a variable named crtc_state, calling container_of on crtc->state It was cleaner to me to have an intermediate variable storing the result of drm_atomic_get_new_crtc_state, but then the most obvious name was taken so I had to rename those two variables before doing so. > The basic idea looks good: > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > But I guess up to the individual driver folks to bikeshed the variable > naming and whatnot. > > One thing I spotted is that a few drivers now gained two aliasing crtc > state pointers in the function: one with the drm type, the other with > a driver specific type. That's something we've outlawed in i915 since > it was making life rather confusing. In i915 we now prefer to use only > the i915 specific types in most places. I didn't spot any of those cases, do you have an example of where it happened? Thanks! Maxime
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel