From: "Jernej Škrabec" <jernej.skrabec@gmail.com>
To: "Ondřej Jirman" <megi@xff.cz>,
"Maxime Ripard" <mripard@kernel.org>,
"Chen-Yu Tsai" <wens@csie.org>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"David Airlie" <airlied@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Samuel Holland" <samuel@sholland.org>,
dri-devel@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
"Jernej Škrabec" <jernej.skrabec@gmail.com>
Subject: Re: [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting
Date: Sat, 24 Feb 2024 08:10:05 +0100 [thread overview]
Message-ID: <6018948.lOV4Wx5bFT@archlinux> (raw)
In-Reply-To: <5h7jcwsdlpe7w2xylbhlw2asfww3znqlmlnszwvvosz5ssokkq@dxhn4v4sy4nq>
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 <megi@xff.cz>
> > > >
> > > > 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 <megi@xff.cz>
> > >
> > > 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 <jernej.skrabec@gmail.com>
> >
> > 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
> > >
> >
> >
> >
> >
>
prev parent reply other threads:[~2024-02-24 7:15 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-16 19:04 [PATCH 0/3] Move blender setup from individual planes to crtc commit in sun4i-drm Ondřej Jirman
2024-02-16 19:04 ` [PATCH 1/3] drm/sun4i: Unify sun8i_*_layer structs Ondřej Jirman
2024-02-21 13:16 ` Maxime Ripard
2024-02-22 19:17 ` Jernej Škrabec
2024-02-16 19:04 ` [PATCH 2/3] drm/sun4i: Add more parameters to sunxi_engine commit callback Ondřej Jirman
2024-02-21 13:17 ` Maxime Ripard
2024-02-22 19:18 ` Jernej Škrabec
2024-02-16 19:04 ` [PATCH 3/3] drm/sun4i: Fix layer zpos change/atomic modesetting Ondřej Jirman
2024-02-21 13:45 ` Maxime Ripard
2024-02-21 14:22 ` Ondřej Jirman
2024-02-22 20:02 ` Jernej Škrabec
2024-02-23 8:20 ` Ondřej Jirman
2024-02-24 2:20 ` Ondřej Jirman
2024-02-24 7:10 ` Jernej Škrabec [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6018948.lOV4Wx5bFT@archlinux \
--to=jernej.skrabec@gmail.com \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=maarten.lankhorst@linux.intel.com \
--cc=megi@xff.cz \
--cc=mripard@kernel.org \
--cc=samuel@sholland.org \
--cc=tzimmermann@suse.de \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).