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

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

[Boris Brezillon]

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.

Thanks.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rob Clark]

On Wed, Feb 4, 2015 at 12:23 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> 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 just wrap drm_atomic_helper_commit() w/ your own fxn that did
extra fixup and then called drm_atomic_helper_commit()?

BR,
-R

> Maybe I'm totally mistaken in my approach to solve this problem, so
> please let me know if you see other solutions.
>
> Thanks.
>
> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Wed, 4 Feb 2015 12:49:19 -0500
Rob Clark <robdclark@gmail.com> wrote:

> On Wed, Feb 4, 2015 at 12:23 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> 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 just wrap drm_atomic_helper_commit() w/ your own fxn that did
> extra fixup and then called drm_atomic_helper_commit()?

Yes, that's something I considered, but this implies modifying/creating
a plane state (calling drm_atomic_get_plane_state and modifying the
discard area fields) after everything has been checked.
If that's fine, then I think I'll go for this solution.

Thanks.

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

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.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Thu, 5 Feb 2015 10:56:30 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> 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) ;-).

[1]https://lkml.org/lkml/2015/2/4/658

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel