[PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Maxime Ripard]

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 at linux.intel.com>
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201103/380b4954/attachment-0001.sig>