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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 05, 2020 at 05:35:28PM +0100, Maxime Ripard wrote:
> On Tue, Nov 03, 2020 at 06:28:24PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote:
> > > 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@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?
> > 
> > eg. ast:
> > +       struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,                               
> > +                                                                         crtc);                               
> >         struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,                           
> >                                                                               crtc);                           
> >         struct ast_private *ast = to_ast_private(crtc->dev);                                                   
> > -       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);                                
> > +       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);       
> > 
> > So here both 'crtc_state' and 'ast_crtc_state' are basically the same
> > thing, which can get a bit confusing especially within larger functions
> > with lots of variables. 
> 
> Ah, that kind of aliasing, ok. So it's purely an issue with
> ergonomics/convenience?

Yeah, I got fed up with it in i915 because I had a hard time
reasoning how the state was being mutated when it was being
accesed through multiple pointers in the same function. And
didn't help that the variable names were all over the place.

> 
> It seems to be a widespread pattern (I know we're using it in vc4 and
> sun4i, and from those reworks I've seen a number of drivers doing so).
> I'm not entirely sure how we should fix it through coccinelle too :/

I think we've been doing these by hand in i915 for the most part.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel