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

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

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. 

In i915 we would just have
struct intel_crtc_state *crtc_state = whatever;
and any access to the base class would be done via crtc_state->base.

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