On Tue, Sep 24, 2019 at 02:40:54PM +0200, Ondřej Jirman wrote:
On first run of the X server, only the black screen and the layer containing the cursor is visible. Switching to console and back corrects the situation.
I have dumped registers, and found out this:
(In both cases there are two enabled planes, plane 1 with zpos 0 and plane 3 with zpos 1).
- First Xorg run:
0x01101000 : 00000201 0x01101080 : 00000030
BLD_FILL_COLOR_CTL: (aka SUN8I_MIXER_BLEND_PIPE_CTL) P1_EN
BLD_CH_RTCTL: (aka SUN8I_MIXER_BLEND_ROUTE) P0_RTCTL channel0 P1_RTCTL channel3
- After switch to console and back to Xorg:
0x01101000 : 00000301 0x01101080 : 00000031
BLD_FILL_COLOR_CTL: P1_EN and P0_EN
BLD_CH_RTCTL: P0_RTCTL channel1 P1_RTCTL channel3
What happens is that sun8i_ui_layer_enable() function may disable blending pipes even if it is no longer assigned to its layer, because of incorrect state/zpos tracking in the driver.
In particular, layer 1 is configured to zpos 0 and thus uses pipe 0. When layer 3 is enabled by X server, sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which will lead to disabling of pipe 0.
In general this issue can happen to any layer during enable or zpos changes on multiple layers at once.
To correct this we now pass previous enabled/disabled state of the layer, and pass real previous zpos of the layer to sun8i_ui_layer_enable() and rework the sun8i_ui_layer_enable() function to react to the state changes correctly. In order to not complicate the atomic_disable callback with all of the above changes, we simply remove it and implement all the chanes as part of atomic_update, which also reduces the code duplication.
I'm not even sure why we need that old state. Can't we just reset completely the whole thing and do the configuration all over again?
That would be nice from a dev standpoint if we can get a complete state for all planes at once from DRM core, but how? DRM helper gives callbacks for updating individual planes with prev and new state. These individual layer change notifications don't map nicely to how pipes are represented in the mixer registers.
You have a pointer to the full DRM state in the state field, so you have that option.
The other option is just to clear everything in atomic_begin, update each plane in atomic_update, and then trigger the readout of the new register values in atomic_commit.
That description just looks to me like the reset is not done properly, and now we have to deal with the fallouts later on.
What in particular?
Having to care about the old state? If the reset was done properly, we wouldn't have to care.
To make this all work, initial zpos positions of all layers need to be restored to initial values on reset.
And this also fixes a whole other bunch of issues, and can be made very trivially in a separate patch.
Maybe reset should also reset registers?
The reset callback actually does the opposite, it resets a DRM state. If anything, we want to initialize the state in reset by reading the registers, not the opposite.
Maxime