Re: [Intel-gfx] [PATCH 5/9] drm/i915: Split skl+ plane update into noarm+arm pair

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

On Mon, Oct 18, 2021 at 08:14:04PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 18, 2021 at 03:06:34PM +0300, Lisovskiy, Stanislav wrote:
> > On Mon, Oct 18, 2021 at 02:50:26PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Chop skl_program_plane() into two halves. Fist half becomes
> > > the _noarm() variant, second part the _arm() variant.
> > > 
> > > Fortunately I have already previously grouped the register
> > > writes into roughtly the correct order, so the split looks
> > > surprisingly clean.
> > > 
> > > A few notable oddities I did not realize were self arming
> > > are AUX_DIST and COLOR_CTL.
> > > 
> > > i915_update_info doesn't look too terrible on my cfl running
> > > kms_atomic_transition --r plane-all-transition --extended:
> > > w/o patch                           w/ patch
> > > Updates: 2178                       Updates: 2018
> > >        |                                   |
> > >    1us |                               1us |
> > >        |                                   |
> > >    4us |                               4us |*****
> > >        |*********                          |**********
> > >   16us |**********                    16us |*******
> > >        |***                                |
> > >   66us |                              66us |
> > >        |                                   |
> > >  262us |                             262us |
> > >        |                                   |
> > >    1ms |                               1ms |
> > >        |                                   |
> > >    4ms |                               4ms |
> > >        |                                   |
> > >   17ms |                              17ms |
> > >        |                                   |
> > > Min update: 8332ns                  Min update: 6164ns
> > > Max update: 48758ns                 Max update: 31808ns
> > > Average update: 19959ns             Average update: 13159ns
> > > Overruns > 100us: 0                 Overruns > 100us: 0
> > > 
> > > And with lockdep enabled:
> > > w/o patch                           w/ patch
> > > Updates: 2177			    Updates: 2172
> > >        |			    	   |
> > >    1us |			       1us |
> > >        |			    	   |
> > >    4us |			       4us |
> > >        |*******			    	   |*********
> > >   16us |**********		      16us |**********
> > >        |*******			    	   |*
> > >   66us |			      66us |
> > >        |			    	   |
> > >  262us |			     262us |
> > >        |			    	   |
> > >    1ms |			       1ms |
> > >        |			    	   |
> > >    4ms |			       4ms |
> > >        |			    	   |
> > >   17ms |			      17ms |
> > >        |			    	   |
> > > Min update: 12645ns		    Min update: 9980ns
> > > Max update: 50153ns		    Max update: 33533ns
> > > Average update: 25337ns		    Average update: 18245ns
> > > Overruns > 250us: 0		    Overruns > 250us: 0
> > > 
> > > TODO: On icl+ everything seems to be armed by PLANE_SURF, so we
> > >       can optimize this even further on modern platforms. But I
> > >       think there's a bit of refactoring to be done first to
> > >       figure out the best way to go about it (eg. just reusing
> > >       the current skl+ functions, or doing a lower level split).
> > > 
> > > TODO: Split scaler programming as well, but IIRC the scaler
> > >       has some oddball double buffering behaviour on some
> > >       platforms, so needs proper reverse engineering
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Should I use that one as a base for further splitting, i.e for DG2?
> > Which refactoring has to be done first, as I understand should be
> > pretty safe to leave only PLANE_SURF update in arm section, and 
> > of course scaler is a different thing.
> 
> I'm not really sure which way we should do the skl+ vs. icl+ split.
> 
> Various ideas I've had:
> - Definitly pull all the icl+ specific things out from the skl+
>   functions and stuff them into icl_plane_update_noarm()
> - After that just call the remaining skl_plane_update_noarm()+arm() 
>   back to back from icl_update_noarm() maybe? I don't like this
>   idea much actually.
> - Maybe instead pull some sequences of register writes into small
>   helpers (eg. colorkey registers could be one). But dunno if there
>   are other clear groups to make this super useful.
> - Or perhaps just pull most fiddly register value calculations 
>   (aux_dist,ckey+alpha things,maybe others) into small helpers
>   to avoid duplicating themm but otherwise fully duplicate all
>   the actual register writes?

I guess that last thing is what I already did with skl_plane_surf()
earlier in the series. So maybe we should just embrace it fully.

-- 
Ville Syrjälä
Intel