Atmel HLCDC + Atomic operations: hook for internal atomic state change

[Daniel Vetter]

On Thu, Feb 05, 2015 at 10:56:30AM +0100, Boris Brezillon wrote:
> Hi Daniel,
> 
> On Thu, 5 Feb 2015 10:34:20 +0100
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Wed, Feb 04, 2015 at 08:58:40PM +0100, Boris Brezillon wrote:
> > > Hi Ville,
> > > 
> > > On Wed, 4 Feb 2015 20:02:27 +0200
> > > Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > > 
> > > > On Wed, Feb 04, 2015 at 06:23:15PM +0100, Boris Brezillon wrote:
> > > > > Hello,
> > > > > 
> > > > > I'm currently adding support for atomic operations (or atomic
> > > > > modesetting) in the Atmel HLCDC driver.
> > > > > Everything is pretty much in place, and all the features provided by the
> > > > > current driver are working as expected.
> > > > > However, there's one feature I'd like to add (actually I was hoping
> > > > > atomic support could help me deal with this feature), and I not sure
> > > > > how to do it.
> > > > > 
> > > > > The HLCDC IP provides a way to discard a specific area on the primary
> > > > > plane (in case at least one of the overlay is activated and alpha
> > > > > blending is disabled).
> > > > > Doing this will reduce the amount of data to transfer from the main
> > > > > memory to the Display Controller, and thus alleviate the load on the
> > > > > memory bus (since this link is quite limited on such hardware,
> > > > > this kind of optimization is really important).
> > > > > 
> > > > > My problem here is that there is no way, in the current atomic
> > > > > implementation, to internally ask for a plane state modification.
> > > > > 
> > > > > Is there a plan to add such hooks that would be called after the
> > > > > requested state modifications (i.e. operations done before the
> > > > > drm_atomic_commit call in all helper functions), but before the atomic
> > > > > checks begin (i.e. call to drm_atomic_check_only) ?
> > > > > Such hooks would let me ask for a primary plane update (modifying the
> > > > > discard area property) if needed.
> > > > > 
> > > > > Maybe I'm totally mistaken in my approach to solve this problem, so
> > > > > please let me know if you see other solutions.
> > > > 
> > > > So this looks pretty much exactly like the overlay optimization feature
> > > > in OMAPs. I don't really see why you need to treat is as some kind of
> > > > plane property. It's just an internal implementation detail so can't you
> > > > just compute the discard area at commit() time based on what planes are
> > > > going to be active? Or if you want to take it into account in some
> > > > bandwidth calculation you can compute it already at check() time.
> > > > 
> > > 
> > > Okay, I'll have a look at the OMAP driver, but I'd really like to
> > > apply the discard area setting as part of the primary plane
> > > atomic_update function (the discard area registers are part of the
> > > primary plane registers, and plane settings are updated by setting a
> > > specific bit to 1).
> > > 
> > > I tried to update the primary plane discard settings as part of the
> > > atomic_update, but when nothing touches the primary plane (an
> > > update_plane on one of the overlay planes), the primary plane is kept
> > > unchanged, and thus the new primary settings are never applied.
> > 
> > So I'm not sure whether I understand this correctly, so let me just invent
> > some fake hw model and explain with that ;-) Please adjust in your reply.
> > 
> > Assumption: We have 1 crtc and 2 planes, a primary and an overlay on top.
> > Our fancy hw has an optional rect within the primary plane which we can
> > tell it not to scan out. The idea is that that rect perfectly matches the
> > placement of the 2nd overlay plane.
> > 
> > Step 1: We need to store this state somewhere of this special rect. So
> > let's create a derived plane state for the primary plane.
> > 
> > struct fhw_primary_plane_state {
> > 	struct drm_plane_state base;
> > 
> > 	bool enable_punchout;
> > 	int punchout_x/_y/_h/_w;
> > };
> > 
> > tegra is a nice example of what you all need to do when your driver needs
> > derived state objects.
> 
> Yep, already created my own state when adding support for atomic
> mode-setting (see [1]), and that's exactly what I was planning to do
> (add disc_x/y/w/h fields in my plane state) ;-).
> 
> > 
> > Step 2: We need to update the state of the _primary_ plane every time the
> > _overlay_ plane moves around or gets enabled/disable. That must be done
> > int the atomic_check hook provided by crtc helpers. Pseudo-code of that
> > functions follows with comments inline
> 
> That's where I was hesitant, so this should be done in the atomic_check.

Well the key bit is that you're allowed to add more state objects. E.g. in
i915 we'll have a global state object for shared dpll, which the core code
will obviously never duplicate for an update. So we'll always do that in
the driver's atomic_check functions (when needed only ofc).
> 
> > 
> > fhw_overlay_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state)
> > {
> > 	/* First we need to get at the state of the primary plane.
> > 	 * Grabbing additional state objects as needed is officially how
> > 	 * ->atomic_check is supposed to work. The locking will magically
> > 	 * work out, as long as you just dutifully pass the unchanged
> > 	 * errno so that deadlock handling is still ok. */
> > 	
> > 	primar_plane = /* exercise for the reader */
> 	primar_plane = state->crtc->primary;
> 
> 	Should work, isn't it ?

Hah, not full score ;-)

If the plane is getting disabled then state->crtc will be NULL. So you
need to look at the current state (in plane->state) and use that crtc.
Also if you allow switching crtcs, then you'd need to look at both of
them, since they could differ.

> > 	primary_plane_state = drm_atomic_get_plane_state(state->state,
> > 							 primary_plane);
> > 	if (IS_ERR(primary_plane_state))
> > 		return PTR_ERR(primary_plane_state);
> > 
> > 	fhw_primary_plane_state = upcast(primary_plane_state);
> > 
> > 	/* Update punchout, only enable when overlay is on. */
> > 	fhw_primary_plane_state.enabel_punchout = !!state->crtc;
> > 	fhw_primary_plane_state.punchout_x = state->crtc_x;
> > 	...
> > 
> > 	return 0;
> > }
> 
> That's exactly what I was planning to do, just wasn't sure if I was
> allowed to modify one of the state when in the atomic_check callback
> (the primary plane might have already been checked, and here, we're
> modifying it afterward).

You're allowed to open-code your check functions, so if you have depencies
like that then:
- Either call the atomic_check for the primary plane directly from the
  overlay plane atomic_check:
- Run the building blocks multiple times (see the individually exported
  atomic_check pieces in the helpers).
- Write your own overall atomic check stuff.

> > Step 3: In your atomic_plane_commit for the _primary_ plane write the
> > punchout rect plus enable bit into hw. Atomic helpers will take care of
> > everything for you. The assumption is that pure plane updates are cheap,
> > so there won't be any optimization for no-op updates. We could add this
> > later on.
> 
> Yep.
> 
> > 
> > Summary: You need three pieces for fancy state:
> > - Your own state structure(s).
> > - Compute that derived state at atomic_check time (totally ok to grab
> >   other states to do this if needed, this is how it's designed).
> > - Bash your special state into hw at commit time.
> 
> Thanks for this detailed answer.

Happy to help out!
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch