Removal of layer properties
Opened this issue · 4 comments
Problem statement
Issue 1
When a layer is de-allocated from a plane the properties are not reset to some default value. That can mean that on next allocation another layer gets allocated to that same plane with a subset of properties not putting other properties in a valid initial state.
Example: alpha property
- Layer L1 is allocated to plane P and sets alpha to 0.1.
- On next allocation L1 is allocated to another layer and some layer L2 not featuring the alpha property is allocated to P.
- Because L2 does not have the alpha property the alpha stays at 0.1, although it should be 1 for L2.
Issue 2
Once a property has been set on a plane it is static as in not removable anymore. Every later layer to plane association must take these properties into account making less optimal layer to plane associations likely over time.
Example: alpha property
- There is a primary plane and one overlay plane. The overlay plane supports the alpha property, the primary doesn't.
- Layer L1 is to be allocated. The continuous buffer stream on L1 could be allocated to any of the two planes.
- But L1 is faded in starting with alpha = 0. libliftoff determines that L1 is put on the overlay plane as only this one supports alpha.
- The alpha of L1 is changed over time from 0 to 1.
- Now an additional layer L2 is to be allocated. L2 can only be allocated to the overlay plane because of buffer formats.
- Although L1 could be put now on the primary plane it won't since the alpha property is still set to some value and L2 can't be put on the primary plane alike. Therefore the layer allocation fails.
The only current way to make a layer less specific in the set of its properties is to destroy and recreate it again (but in this case we do struggle with issue 1 again). I would argue it makes the API more intuitive if there is a way to "remove" properties again from a layer instead of only destroying the whole layer.
Solution plan
I have some ideas how to solve these issues but before formulating it out I would like to get some feedback if the two issues above are indeed problematic or if I overlooked something.
Introduce a plane property defunct state
When a property P of a plane PL was previously set in allocations A1...AN with N > 0 and is not any more set at the next allocation AN+1 property P goes into a defunct state. That means at allocation AN+1 property P is "reset" to its default value if such a default exists for property P. For that plane PL is forced into the next atomic commit independent of its allocation in AN+1.
Introduce a layer property default setter
The default value of property P used above for the plane property defunct state must be set explicitly. For that the compositor can call a function
liftoff_layer_set_property_with_default(struct liftoff_layer *layer,
const char *name,
uint64_t value,
uint64_t default)
Introduce wrappers for common properties
That default setter above is cumbersome. For common properties there should be wrappers that call liftoff_layer_set_property_with_default with the correct default values. For example for alpha:
liftoff_layer_set_property_alpha(struct liftoff_layer *layer, uint64_t value)
Introduce a property remove function
For upcoming allocations layer L can have a property removed by calling some function on it.
Thanks for the detailed issue, that really helps! I agree we need to fix both of these issues.
Issue 1 is pretty complicated. It's been a problem for a while, for instance VT switching requires compositors to reset properties they don't necessarily understand. See https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
We could save the initial values of the plane properties, keep track of which ones we've changed, and reset those when disabling (or re-using) a plane. This would rely on the initial value of the plane properties to be sane enough though.
It would be very nice to have a way to ask KMS to reset a plane property to its default value (or to make the kernel expose it somehow, via drmModePropertyRes?).
Otherwise, the library user will have to provide a default value indeed ("Introduce a property default setter").
The plan for issue 2 so far was to continue what we do for the zpos property right now. The user sets zpos, and we emulate the property when it's not exposed by the driver. (On top of that, we finding planes for layers, we're allowing zpos changes as long as the relative position between layers is unchanged.)
So for the alpha property, this would mean:
- If the plane has a alpha property, use that
- Otherwise:
- If the layer has alpha set to 1.0, ignore the layer alpha property
- If the layer has alpha set to something else, don't put the layer into this plane
The nice thing about this is that compositors don't have to worry about using properties that may be unsupported - they can just set zpos (and alpha) without caring about it. libliftoff will use planes if possible, and fallback to composition if necessary.
We can do this for all standard properties. I'd rather not add support for driver-specific properties, which are frozen in KMS anyway (no new driver-specific props can be added to any driver right now).
Introduce a plane property defunct state
Can you expand on that? I'm not sure I understand how this would work.
Introduce wrappers for common properties
I think that's pretty similar to the solution outlined above (adding special support for standard props), except libliftoff does strcmp(prop_name, "alpha") instead of having e.g. liftoff_layer_set_alpha.
Introduce a property remove function
That may be useful even if other solutions are implemented. I've left this solution out so far because I didn't have a use-case for it. I figured compositors would just set alpha to 1.0 when they need to reset it.
I have added now some more info in the first post. Check it out and tell me if you think the plan is sound.
Thanks for the detailed issue, that really helps! I agree we need to fix both of these issues.
Issue 1 is pretty complicated. It's been a problem for a while, for instance VT switching requires compositors to reset properties they don't necessarily understand. See https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
We could save the initial values of the plane properties, keep track of which ones we've changed, and reset those when disabling (or re-using) a plane. This would rely on the initial value of the plane properties to be sane enough though.
It would be very nice to have a way to ask KMS to reset a plane property to its default value (or to make the kernel expose it somehow, via
drmModePropertyRes?).Otherwise, the library user will have to provide a default value indeed ("Introduce a property default setter").
Yes, that's how I see it too. I have added a plan for the second variant above. You can of course also look into making the kernel provide us with a default value.
The plan for issue 2 so far was to continue what we do for the
zposproperty right now. The user setszpos, and we emulate the property when it's not exposed by the driver. (On top of that, we finding planes for layers, we're allowing zpos changes as long as the relative position between layers is unchanged.)So for the alpha property, this would mean:
If the plane has a alpha property, use that
Otherwise:
- If the layer has alpha set to 1.0, ignore the layer alpha property
- If the layer has alpha set to something else, don't put the layer into this plane
The nice thing about this is that compositors don't have to worry about using properties that may be unsupported - they can just set zpos (and alpha) without caring about it. libliftoff will use planes if possible, and fallback to composition if necessary.
We can do this for all standard properties. I'd rather not add support for driver-specific properties, which are frozen in KMS anyway (no new driver-specific props can be added to any driver right now).
Ignoring alpha when set to 1.0 is similar to having a default value. I think above plan aligns with that requirement.
Introduce a plane property defunct state
Can you expand on that? I'm not sure I understand how this would work.
I have added a description above now. The idea is basically to force a property reset on a plane if the property is not set anymore. For that we need to track prior property values. I know libliftoff is build on the premise that allocations are independent, so this could be tricky. But it should be only a relation from one allocation to the next one. Maybe on plane reset at the beginning of the allocation?
What's not solved by this mechanism are of course manipulated properties on VT switches the compositor and the library have no knowledge of.
Introduce wrappers for common properties
I think that's pretty similar to the solution outlined above (adding special support for standard props), except libliftoff does
strcmp(prop_name, "alpha")instead of having e.g.liftoff_layer_set_alpha.Introduce a property remove function
That may be useful even if other solutions are implemented. I've left this solution out so far because I didn't have a use-case for it. I figured compositors would just set alpha to 1.0 when they need to reset it.
Yea, this would work too when we have an "ignore-property-alpha-when-default-value" logic.
This thread discusses about exposing default KMS property values: https://lists.freedesktop.org/archives/dri-devel/2020-April/263290.html
The consensus is that the kernel won't expose default KMS property values. User-space needs to come up with its own solution.
Introduce a plane property defunct state
Maybe on plane reset at the beginning of the allocation?
Sounds reasonable.
Introduce a layer property default setter
Maybe setting the default could be done only once on compositor startup?
void liftoff_device_set_property_default(struct liftoff_device *device, const char *name, uint64_t value);(And then we can keep liftoff_layer_set_property as-is.)