[PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Daniel Vetter]

On Tue, Feb 11, 2020 at 04:40:21PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 03:00:30PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
> > > Hi Ville,
> > > 
> > > On 10/02/2020 18:03, Ville Syrjälä wrote:
> > > 
> > > > The usual approach we follow in i915 for things that affect more
> > > > than one plane is is to collect that state into the crtc state.
> > > > That way we get to remember it for the planes that are not part
> > > > of the current commit.
> > > > 
> > > > And when we have state that affects more than one crtc that again
> > > > get collected up one level up in what we call global state
> > > > (basically drm_private_obj with less heavy handed locking scheme).
> > > 
> > > I'm confused. Don't we always have the full state available? Why do you need to store state into 
> > > custom crtc-state?
> > > 
> > > Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
> > > state object and duplicating that information there seems a bit silly, as surely that information is 
> > > tracked by DRM?
> > 
> > You can have it if you add all the planes to the state, which can be
> > a bit expensive. Another option would to peek into the planes' states
> > that aren't in the commit, but that's quite gross due to bypassing
> > the normal locking rules and instead relying on the crtc mutex to
> > sufficiently protect the plane states as well. And I suspect trying
> > to do said peeking during the commit phase when the locks have
> > already been dropped will end badly.
> 
> Yup, don't peek outside of atomic_check.
> 
> Also the peeking only works for planes associated to the crtc. Either
> because that's how the hw works (i915 has fixed plane routing).
> 
> Now if this is only about all the planes currently active on a crtc, then
> you the helpers will already add all those plane states for you, and you
> can just walk them in your commit function. Not exactly sure what you need
> here.

See drm_atomic_add_affected_planes() in case you're rolling your own
stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch