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@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,

Well, I don't know if they actually implement it. I just recall implementing it years ago for the n900, but I have the recollection that it never went upstream.

Hi Daniel,

On Thu, 5 Feb 2015 10:34:20 +0100 Daniel Vetter daniel@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@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.

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 ?

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).

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.

Best Regards,

Boris

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@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@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