From: Ondrej Jirman <[email protected]>
Identical configurations of planes can lead to different (and wrong)
layer -> pipe routing at HW level, depending on the order of atomic
plane changes.
For example:
- Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
is enabled. This is a typical situation at boot.
- When a compositor takes over and layer 3 is enabled,
sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
What happens is that sun8i_ui_layer_enable() function may disable
blender pipes even if it is no longer assigned to its layer.
To correct this, move the routing setup out of individual plane's
atomic_update into crtc's atomic_update, where it can be calculated
and updated all at once.
Remove the atomic_disable callback because it is no longer needed.
Signed-off-by: Ondrej Jirman <[email protected]>
---
drivers/gpu/drm/sun4i/sun8i_mixer.c | 73 +++++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun8i_mixer.h | 6 +++
drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 73 +------------------------
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 74 +-------------------------
4 files changed, 83 insertions(+), 143 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index bdeb9b80e038..21331d4ffe01 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -250,12 +250,85 @@ int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format)
return -EINVAL;
}
+static void sun8i_layer_enable(struct sun8i_layer *layer, bool enable)
+{
+ u32 ch_base = sun8i_channel_base(layer->mixer, layer->channel);
+ u32 val, reg, mask;
+
+ if (layer->type == SUN8I_LAYER_TYPE_UI) {
+ val = enable ? SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN : 0;
+ mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
+ reg = SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, layer->overlay);
+ } else {
+ val = enable ? SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN : 0;
+ mask = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
+ reg = SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, layer->overlay);
+ }
+
+ regmap_update_bits(layer->mixer->engine.regs, reg, mask, val);
+}
+
static void sun8i_mixer_commit(struct sunxi_engine *engine,
struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+ u32 bld_base = sun8i_blender_base(mixer);
+ struct drm_plane_state *plane_state;
+ struct drm_plane *plane;
+ u32 route = 0, pipe_en = 0;
+
DRM_DEBUG_DRIVER("Committing changes\n");
+ drm_for_each_plane(plane, state->dev) {
+ struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
+ bool enable;
+ int zpos;
+
+ if (!(plane->possible_crtcs & drm_crtc_mask(crtc)) || layer->mixer != mixer)
+ continue;
+
+ plane_state = drm_atomic_get_new_plane_state(state, plane);
+ if (!plane_state)
+ plane_state = plane->state;
+
+ enable = plane_state->crtc && plane_state->visible;
+ zpos = plane_state->normalized_zpos;
+
+ DRM_DEBUG_DRIVER(" plane %d: chan=%d ovl=%d en=%d zpos=%d\n",
+ plane->base.id, layer->channel, layer->overlay,
+ enable, zpos);
+
+ /*
+ * We always update the layer enable bit, because it can clear
+ * spontaneously for unknown reasons.
+ */
+ sun8i_layer_enable(layer, enable);
+
+ if (!enable)
+ continue;
+
+ /* Route layer to pipe based on zpos */
+ route |= layer->channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
+ pipe_en |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
+ }
+
+ regmap_update_bits(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_ROUTE(bld_base),
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
+ route);
+
+ regmap_update_bits(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
+ pipe_en);
+
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
}
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index 5a610ee30301..d7898c9c9cc0 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -186,9 +186,15 @@ struct sun8i_mixer {
struct clk *mod_clk;
};
+enum {
+ SUN8I_LAYER_TYPE_UI,
+ SUN8I_LAYER_TYPE_VI,
+};
+
struct sun8i_layer {
struct drm_plane plane;
struct sun8i_mixer *mixer;
+ int type;
int channel;
int overlay;
};
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 248fbb606ede..b90e5edef4e8 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -24,55 +24,6 @@
#include "sun8i_ui_layer.h"
#include "sun8i_ui_scaler.h"
-static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
- int overlay, bool enable, unsigned int zpos,
- unsigned int old_zpos)
-{
- u32 val, bld_base, ch_base;
-
- bld_base = sun8i_blender_base(mixer);
- ch_base = sun8i_channel_base(mixer, channel);
-
- DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
- enable ? "En" : "Dis", channel, overlay);
-
- if (enable)
- val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
- else
- val = 0;
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
- SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
-
- if (!enable || zpos != old_zpos) {
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
- SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
- 0);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_ROUTE(bld_base),
- SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
- 0);
- }
-
- if (enable) {
- val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
- val, val);
-
- val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_ROUTE(bld_base),
- SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
- val);
- }
-}
-
static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
int overlay, struct drm_plane *plane)
{
@@ -259,36 +210,18 @@ static int sun8i_ui_layer_atomic_check(struct drm_plane *plane,
true, true);
}
-static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
- struct drm_atomic_state *state)
-{
- struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
- plane);
- struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
- unsigned int old_zpos = old_state->normalized_zpos;
- struct sun8i_mixer *mixer = layer->mixer;
-
- sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
- old_zpos);
-}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
{
- struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
- plane);
struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
plane);
struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
unsigned int zpos = new_state->normalized_zpos;
- unsigned int old_zpos = old_state->normalized_zpos;
struct sun8i_mixer *mixer = layer->mixer;
- if (!new_state->visible) {
- sun8i_ui_layer_enable(mixer, layer->channel,
- layer->overlay, false, 0, old_zpos);
+ if (!new_state->crtc || !new_state->visible)
return;
- }
sun8i_ui_layer_update_coord(mixer, layer->channel,
layer->overlay, plane, zpos);
@@ -298,13 +231,10 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
layer->overlay, plane);
sun8i_ui_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane);
- sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
- true, zpos, old_zpos);
}
static const struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
.atomic_check = sun8i_ui_layer_atomic_check,
- .atomic_disable = sun8i_ui_layer_atomic_disable,
.atomic_update = sun8i_ui_layer_atomic_update,
};
@@ -390,6 +320,7 @@ struct sun8i_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
drm_plane_helper_add(&layer->plane, &sun8i_ui_layer_helper_funcs);
layer->mixer = mixer;
+ layer->type = SUN8I_LAYER_TYPE_UI;
layer->channel = channel;
layer->overlay = 0;
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 0c0f1ac80517..9c09d9c08496 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -18,55 +18,6 @@
#include "sun8i_vi_layer.h"
#include "sun8i_vi_scaler.h"
-static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
- int overlay, bool enable, unsigned int zpos,
- unsigned int old_zpos)
-{
- u32 val, bld_base, ch_base;
-
- bld_base = sun8i_blender_base(mixer);
- ch_base = sun8i_channel_base(mixer, channel);
-
- DRM_DEBUG_DRIVER("%sabling VI channel %d overlay %d\n",
- enable ? "En" : "Dis", channel, overlay);
-
- if (enable)
- val = SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN;
- else
- val = 0;
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
- SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
-
- if (!enable || zpos != old_zpos) {
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
- SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
- 0);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_ROUTE(bld_base),
- SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
- 0);
- }
-
- if (enable) {
- val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
- val, val);
-
- val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
-
- regmap_update_bits(mixer->engine.regs,
- SUN8I_MIXER_BLEND_ROUTE(bld_base),
- SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
- val);
- }
-}
-
static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
int overlay, struct drm_plane *plane)
{
@@ -393,36 +344,17 @@ static int sun8i_vi_layer_atomic_check(struct drm_plane *plane,
true, true);
}
-static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
- struct drm_atomic_state *state)
-{
- struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
- plane);
- struct sun8i_layer *layer = plane_to_sun8i_vi_layer(plane);
- unsigned int old_zpos = old_state->normalized_zpos;
- struct sun8i_mixer *mixer = layer->mixer;
-
- sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0,
- old_zpos);
-}
-
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
{
- struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
- plane);
struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
plane);
struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
unsigned int zpos = new_state->normalized_zpos;
- unsigned int old_zpos = old_state->normalized_zpos;
struct sun8i_mixer *mixer = layer->mixer;
- if (!new_state->visible) {
- sun8i_vi_layer_enable(mixer, layer->channel,
- layer->overlay, false, 0, old_zpos);
+ if (!new_state->crtc || !new_state->visible)
return;
- }
sun8i_vi_layer_update_coord(mixer, layer->channel,
layer->overlay, plane, zpos);
@@ -432,13 +364,10 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
layer->overlay, plane);
sun8i_vi_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane);
- sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
- true, zpos, old_zpos);
}
static const struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
.atomic_check = sun8i_vi_layer_atomic_check,
- .atomic_disable = sun8i_vi_layer_atomic_disable,
.atomic_update = sun8i_vi_layer_atomic_update,
};
@@ -613,6 +542,7 @@ struct sun8i_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
drm_plane_helper_add(&layer->plane, &sun8i_vi_layer_helper_funcs);
layer->mixer = mixer;
+ layer->type = SUN8I_LAYER_TYPE_VI;
layer->channel = index;
layer->overlay = 0;
--
2.43.0
Hi,
On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> From: Ondrej Jirman <[email protected]>
>
> Identical configurations of planes can lead to different (and wrong)
> layer -> pipe routing at HW level, depending on the order of atomic
> plane changes.
>
> For example:
>
> - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> is enabled. This is a typical situation at boot.
>
> - When a compositor takes over and layer 3 is enabled,
> sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
>
> What happens is that sun8i_ui_layer_enable() function may disable
> blender pipes even if it is no longer assigned to its layer.
>
> To correct this, move the routing setup out of individual plane's
> atomic_update into crtc's atomic_update, where it can be calculated
> and updated all at once.
>
> Remove the atomic_disable callback because it is no longer needed.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
I don't have enough knowledge about the mixers code to comment on your
patch, so I'll let Jernej review it. However, this feels to me like the
pipe assignment is typically the sort of things that should be dealt
with device-wide, and in atomic_check.
If I'm talking non-sense, it would be great to mention at least why that
can't be an option in the commit log.
Maxime
Hi Maxime,
On Wed, Feb 21, 2024 at 02:45:20PM +0100, Maxime Ripard wrote:
> Hi,
>
> On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > From: Ondrej Jirman <[email protected]>
> >
> > Identical configurations of planes can lead to different (and wrong)
> > layer -> pipe routing at HW level, depending on the order of atomic
> > plane changes.
> >
> > For example:
> >
> > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > is enabled. This is a typical situation at boot.
> >
> > - When a compositor takes over and layer 3 is enabled,
> > sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> >
> > What happens is that sun8i_ui_layer_enable() function may disable
> > blender pipes even if it is no longer assigned to its layer.
> >
> > To correct this, move the routing setup out of individual plane's
> > atomic_update into crtc's atomic_update, where it can be calculated
> > and updated all at once.
> >
> > Remove the atomic_disable callback because it is no longer needed.
> >
> > Signed-off-by: Ondrej Jirman <[email protected]>
>
> I don't have enough knowledge about the mixers code to comment on your
> patch, so I'll let Jernej review it. However, this feels to me like the
> pipe assignment is typically the sort of things that should be dealt
> with device-wide, and in atomic_check.
Yes, this is what this patch does. It moves blender setup from individual
planes' attomic commits to a single place in crtc atomic commit.
kind regards,
o.
> If I'm talking non-sense, it would be great to mention at least why that
> can't be an option in the commit log.
>
> Maxime
Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> Hi,
>
> On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > From: Ondrej Jirman <[email protected]>
> >
> > Identical configurations of planes can lead to different (and wrong)
> > layer -> pipe routing at HW level, depending on the order of atomic
> > plane changes.
> >
> > For example:
> >
> > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > is enabled. This is a typical situation at boot.
> >
> > - When a compositor takes over and layer 3 is enabled,
> > sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> >
> > What happens is that sun8i_ui_layer_enable() function may disable
> > blender pipes even if it is no longer assigned to its layer.
> >
> > To correct this, move the routing setup out of individual plane's
> > atomic_update into crtc's atomic_update, where it can be calculated
> > and updated all at once.
> >
> > Remove the atomic_disable callback because it is no longer needed.
> >
> > Signed-off-by: Ondrej Jirman <[email protected]>
>
> I don't have enough knowledge about the mixers code to comment on your
> patch, so I'll let Jernej review it. However, this feels to me like the
> pipe assignment is typically the sort of things that should be dealt
> with device-wide, and in atomic_check.
In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
one is hardwired to specific mixer. Movable planes are the feature of DE3.3
and one of the pain points for upstreaming the code. Anyway, this commit only
addresses current issue of enabling and disabling planes and handling zpos.
In atomic check you can only precalculate final register values, but I don't
see any benefit doing that. I think that this code elegantly solves current
issue of enabling or disabling wrong plane in certain situations, so:
Reviewed-by: Jernej Skrabec <[email protected]>
Note, if there is new revision, please rewrite blender regmap_update_bits()
to regmap_write(). Since there is HW issue with reads, I would like to
get rid of regmap_update_bits() calls eventually.
Best regards,
Jernej
>
> If I'm talking non-sense, it would be great to mention at least why that
> can't be an option in the commit log.
>
> Maxime
>
On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > Hi,
> >
> > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > From: Ondrej Jirman <[email protected]>
> > >
> > > Identical configurations of planes can lead to different (and wrong)
> > > layer -> pipe routing at HW level, depending on the order of atomic
> > > plane changes.
> > >
> > > For example:
> > >
> > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > > is enabled. This is a typical situation at boot.
> > >
> > > - When a compositor takes over and layer 3 is enabled,
> > > sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > > will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > >
> > > What happens is that sun8i_ui_layer_enable() function may disable
> > > blender pipes even if it is no longer assigned to its layer.
> > >
> > > To correct this, move the routing setup out of individual plane's
> > > atomic_update into crtc's atomic_update, where it can be calculated
> > > and updated all at once.
> > >
> > > Remove the atomic_disable callback because it is no longer needed.
> > >
> > > Signed-off-by: Ondrej Jirman <[email protected]>
> >
> > I don't have enough knowledge about the mixers code to comment on your
> > patch, so I'll let Jernej review it. However, this feels to me like the
> > pipe assignment is typically the sort of things that should be dealt
> > with device-wide, and in atomic_check.
>
> In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> and one of the pain points for upstreaming the code. Anyway, this commit only
> addresses current issue of enabling and disabling planes and handling zpos.
>
> In atomic check you can only precalculate final register values, but I don't
> see any benefit doing that. I think that this code elegantly solves current
> issue of enabling or disabling wrong plane in certain situations, so:
>
> Reviewed-by: Jernej Skrabec <[email protected]>
Thanks for the review.
> Note, if there is new revision, please rewrite blender regmap_update_bits()
> to regmap_write(). Since there is HW issue with reads, I would like to
> get rid of regmap_update_bits() calls eventually.
BTW as a side note, one observation I made about a year back after some testing
with a setup like this:
- PIN photodiode in a black box directed at a display (Pinephone)
attached to the oscilloscope
- GPIO signal routed to another input of the scope
- sun4i-drm driver modification that toggles the GPIO any number of times so
that I can correlate brightness changes as seen by photodiode on the
scope with the timing of what's happening in the driver
is that if you do atomic commit right after receiving the flip event there's
a high chance that what you're commiting will get displayed right away,
basically skipping the update from the previous atomic commit.
It's very easy to reproduce with a test application that flips between two
completely white and completely black framebuffers back and forth right
after flip event. What you see on the display is seemingly random blinking
pattern: https://megous.com/dl/tmp/c9a286427ce66d80.png when it should be
a peridic flipping between black and white pixels like this
https://megous.com/dl/tmp/c085a50ccd387257.png
This goes away for me when introducing a >150us delay between flip event
and next atomic commit.
I did a more complex test with a test app that switches between 4 framebuffers
of decreasing shades of gray, so that the resulting pattern of brightness should
be a stariwise stepping signal pattern on the scope, along with gpio indicating which
framebuffer just got commited (by the number of pulses 1-4). What I saw was
that normally there's a 1 frame delay between commit and the corresponding
brightness change, but often enough the brightness change is immediate.
I think the hardware indicates a flip (vsync irq) some time before it performs
the double buffered register update to shadow register for the next frame. So
sometimes the driver manages to overwrite the values stored during the previous
commit before they get flipped to shadow registers. Again, intorducing a 150us
delay between flip event and atomic commit reliably fixes this, and I see
a perfect stairwise pattern on the scope instead of occasional skipped ahead
steps.
It seems to me that those 150us are related to display timing.
Vertical blanking cycle on Pinephone lasts about 400us with 45us long vsync
pulse roughly in the middle. In my observation it seems likely that HW signals
VSYNC interrupt at the start or end of the VSYNC pulse and flips the double
buffered HW registers at the end of the blanking cycle, leaving some 150us of
time in between for a quick atomic commit to still mess up the register settings
for the upcoming scan out, before they get commited to shadow regs. I don't have
a way to observe vsync signal on a scope, because it's MIPI-DSI display to
verify this exactly.
The certain thing is that after vsync interrupt, the double buffered registers
are not switched, yet. There's still some time left before that happens.
Anyway, just something I'd thought I share that may help us understand this
HW better. :)
kind regards,
o.
> Best regards,
> Jernej
>
> >
> > If I'm talking non-sense, it would be great to mention at least why that
> > can't be an option in the commit log.
> >
> > Maxime
> >
>
>
>
>
On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > Hi,
> >
> > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > From: Ondrej Jirman <[email protected]>
> > >
> > > Identical configurations of planes can lead to different (and wrong)
> > > layer -> pipe routing at HW level, depending on the order of atomic
> > > plane changes.
> > >
> > > For example:
> > >
> > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > > is enabled. This is a typical situation at boot.
> > >
> > > - When a compositor takes over and layer 3 is enabled,
> > > sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > > will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > >
> > > What happens is that sun8i_ui_layer_enable() function may disable
> > > blender pipes even if it is no longer assigned to its layer.
> > >
> > > To correct this, move the routing setup out of individual plane's
> > > atomic_update into crtc's atomic_update, where it can be calculated
> > > and updated all at once.
> > >
> > > Remove the atomic_disable callback because it is no longer needed.
> > >
> > > Signed-off-by: Ondrej Jirman <[email protected]>
> >
> > I don't have enough knowledge about the mixers code to comment on your
> > patch, so I'll let Jernej review it. However, this feels to me like the
> > pipe assignment is typically the sort of things that should be dealt
> > with device-wide, and in atomic_check.
>
> In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> and one of the pain points for upstreaming the code. Anyway, this commit only
> addresses current issue of enabling and disabling planes and handling zpos.
>
> In atomic check you can only precalculate final register values, but I don't
> see any benefit doing that. I think that this code elegantly solves current
> issue of enabling or disabling wrong plane in certain situations, so:
>
> Reviewed-by: Jernej Skrabec <[email protected]>
>
> Note, if there is new revision, please rewrite blender regmap_update_bits()
> to regmap_write(). Since there is HW issue with reads, I would like to
> get rid of regmap_update_bits() calls eventually.
I've looked into it and I can probably rewrite these quite readily:
+ regmap_update_bits(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_ROUTE(bld_base),
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
+ SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
+ route);
The mask here covers all implemented bits in the register.
+ regmap_update_bits(mixer->engine.regs,
+ SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
+ pipe_en);
+
The mask here doesn't cover BLD_FILL_COLOR_CTL.Px_FCEN bits that implement solid
color filling. But those can be 0 anyway except for pipe0 which is hardcoded by
the driver to 1, I think:
631 /*
632 * Set fill color of bottom plane to black. Generally not needed
633 * except when VI plane is at bottom (zpos = 0) and enabled.
634 */
635 regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
636 SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
I will not be able to get rid of regmap_update_bits in sun8i_layer_enable
because that register there has other important things in it like framebuffer
pixel format, etc.
kind regards,
o.
> Best regards,
> Jernej
>
> >
> > If I'm talking non-sense, it would be great to mention at least why that
> > can't be an option in the commit log.
> >
> > Maxime
> >
>
>
>
>
On Saturday, February 24, 2024 3:20:43 AM CET Ondřej Jirman wrote:
> On Thu, Feb 22, 2024 at 09:02:53PM +0100, Jernej Škrabec wrote:
> > Dne sreda, 21. februar 2024 ob 14:45:20 CET je Maxime Ripard napisal(a):
> > > Hi,
> > >
> > > On Fri, Feb 16, 2024 at 08:04:26PM +0100, Ondřej Jirman wrote:
> > > > From: Ondrej Jirman <[email protected]>
> > > >
> > > > Identical configurations of planes can lead to different (and wrong)
> > > > layer -> pipe routing at HW level, depending on the order of atomic
> > > > plane changes.
> > > >
> > > > For example:
> > > >
> > > > - Layer 1 is configured to zpos 0 and thus uses pipe 0. No other layer
> > > > is enabled. This is a typical situation at boot.
> > > >
> > > > - When a compositor takes over and layer 3 is enabled,
> > > > sun8i_ui_layer_enable() will get called with old_zpos=0 zpos=1, which
> > > > will lead to incorrect disabling of pipe 0 and enabling of pipe 1.
> > > >
> > > > What happens is that sun8i_ui_layer_enable() function may disable
> > > > blender pipes even if it is no longer assigned to its layer.
> > > >
> > > > To correct this, move the routing setup out of individual plane's
> > > > atomic_update into crtc's atomic_update, where it can be calculated
> > > > and updated all at once.
> > > >
> > > > Remove the atomic_disable callback because it is no longer needed.
> > > >
> > > > Signed-off-by: Ondrej Jirman <[email protected]>
> > >
> > > I don't have enough knowledge about the mixers code to comment on your
> > > patch, so I'll let Jernej review it. However, this feels to me like the
> > > pipe assignment is typically the sort of things that should be dealt
> > > with device-wide, and in atomic_check.
> >
> > In DE2 and DE3.0, you cannot move planes between mixers (crtcs), because each
> > one is hardwired to specific mixer. Movable planes are the feature of DE3.3
> > and one of the pain points for upstreaming the code. Anyway, this commit only
> > addresses current issue of enabling and disabling planes and handling zpos.
> >
> > In atomic check you can only precalculate final register values, but I don't
> > see any benefit doing that. I think that this code elegantly solves current
> > issue of enabling or disabling wrong plane in certain situations, so:
> >
> > Reviewed-by: Jernej Skrabec <[email protected]>
> >
> > Note, if there is new revision, please rewrite blender regmap_update_bits()
> > to regmap_write(). Since there is HW issue with reads, I would like to
> > get rid of regmap_update_bits() calls eventually.
>
> I've looked into it and I can probably rewrite these quite readily:
>
> + regmap_update_bits(mixer->engine.regs,
> + SUN8I_MIXER_BLEND_ROUTE(bld_base),
> + SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(0) |
> + SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(1) |
> + SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(2) |
> + SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(3),
> + route);
>
> The mask here covers all implemented bits in the register.
>
> + regmap_update_bits(mixer->engine.regs,
> + SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> + SUN8I_MIXER_BLEND_PIPE_CTL_EN(0) |
> + SUN8I_MIXER_BLEND_PIPE_CTL_EN(1) |
> + SUN8I_MIXER_BLEND_PIPE_CTL_EN(2) |
> + SUN8I_MIXER_BLEND_PIPE_CTL_EN(3),
> + pipe_en);
> +
>
> The mask here doesn't cover BLD_FILL_COLOR_CTL.Px_FCEN bits that implement solid
> color filling. But those can be 0 anyway except for pipe0 which is hardcoded by
> the driver to 1, I think:
>
> 631 /*
> 632 * Set fill color of bottom plane to black. Generally not needed
> 633 * except when VI plane is at bottom (zpos = 0) and enabled.
> 634 */
> 635 regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
> 636 SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0));
Correct on all counts. That's what I've meant.
>
> I will not be able to get rid of regmap_update_bits in sun8i_layer_enable
> because that register there has other important things in it like framebuffer
> pixel format, etc.
Yeah, this rework would certainly be more involved, so it's out of the scope of
this series.
Best regards,
Jernej
>
> kind regards,
> o.
>
> > Best regards,
> > Jernej
> >
> > >
> > > If I'm talking non-sense, it would be great to mention at least why that
> > > can't be an option in the commit log.
> > >
> > > Maxime
> > >
> >
> >
> >
> >
>
