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

[Daniel Vetter]

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.

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

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 */
	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;
}

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.

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.

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