According to DE2.0/DE3.0 manual VI scaler enable register is double
buffered, but de facto it doesn't, or the hardware has the shadow
register latching issues which causes single-frame picture corruption
after changing the state of scaler enable register.
Allow the user to keep the scaler always enabled, preventing the UI
glitches on the transition from scaled to unscaled state.
NOTE:
UI layer scaler has more registers with double-buffering issue and can't
be workarounded in the same manner.
You may find a python test and a demo video for this issue at [1]
[1]: https://github.com/GloDroid/glodroid_tests/issues/4
Signed-off-by: Roman Stratiienko <[email protected]>
---
drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 71ab0a00b4de..15cad0330f66 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -27,6 +27,18 @@
#include "sun8i_vi_layer.h"
#include "sunxi_engine.h"
+/* According to DE2.0/DE3.0 manual VI scaler enable register is double
+ * buffered, but de facto it doesn't, or the hardware has the shadow
+ * register latching issues which causes single-frame picture corruption
+ * after changing the state of scaler enable register.
+ * Allow the user to keep the scaler always enabled, preventing the UI
+ * glitches on the transition from scaled to unscaled state.
+ */
+int sun8i_vi_keep_scaler_enabled;
+MODULE_PARM_DESC(keep_vi_scaler_enabled,
+ "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
+module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
+
struct de2_fmt_info {
u32 drm_fmt;
u32 de2_fmt;
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 662ba1018cc4..f005ab883503 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -17,6 +17,8 @@
#include "sun8i_vi_layer.h"
#include "sun8i_vi_scaler.h"
+extern int sun8i_vi_keep_scaler_enabled;
+
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable, unsigned int zpos)
{
@@ -149,7 +151,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
*/
subsampled = format->hsub > 1 || format->vsub > 1;
- if (insize != outsize || subsampled || hphase || vphase) {
+ if (insize != outsize || subsampled || hphase || vphase || sun8i_vi_keep_scaler_enabled) {
unsigned int scanline, required;
struct drm_display_mode *mode;
u32 hscale, vscale, fps;
--
2.30.2
Hi Roman,
On Thu, Jun 02, 2022 at 06:01:18PM +0000, Roman Stratiienko wrote:
> According to DE2.0/DE3.0 manual VI scaler enable register is double
> buffered, but de facto it doesn't, or the hardware has the shadow
> register latching issues which causes single-frame picture corruption
> after changing the state of scaler enable register.
>
> Allow the user to keep the scaler always enabled, preventing the UI
> glitches on the transition from scaled to unscaled state.
>
> NOTE:
> UI layer scaler has more registers with double-buffering issue and can't
> be workarounded in the same manner.
>
> You may find a python test and a demo video for this issue at [1]
Isn't this an issue with kernel driver not waiting for DE2 FINISH IRQ, but
for VBLANK IRQ from TCON instead, before allowing to write new set of register
values?
https://megous.com/dl/tmp/4fe35b3fc72ee7de.png
I haven't checked if FINISH flag is set at time of VBLANK interrupt, so maybe
this is not the issue.
regards,
o.
> [1]: https://github.com/GloDroid/glodroid_tests/issues/4
> Signed-off-by: Roman Stratiienko <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 71ab0a00b4de..15cad0330f66 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -27,6 +27,18 @@
> #include "sun8i_vi_layer.h"
> #include "sunxi_engine.h"
>
> +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> + * buffered, but de facto it doesn't, or the hardware has the shadow
> + * register latching issues which causes single-frame picture corruption
> + * after changing the state of scaler enable register.
> + * Allow the user to keep the scaler always enabled, preventing the UI
> + * glitches on the transition from scaled to unscaled state.
> + */
> +int sun8i_vi_keep_scaler_enabled;
> +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> + "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
> +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
> +
> struct de2_fmt_info {
> u32 drm_fmt;
> u32 de2_fmt;
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index 662ba1018cc4..f005ab883503 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -17,6 +17,8 @@
> #include "sun8i_vi_layer.h"
> #include "sun8i_vi_scaler.h"
>
> +extern int sun8i_vi_keep_scaler_enabled;
> +
> static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
> int overlay, bool enable, unsigned int zpos)
> {
> @@ -149,7 +151,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> */
> subsampled = format->hsub > 1 || format->vsub > 1;
>
> - if (insize != outsize || subsampled || hphase || vphase) {
> + if (insize != outsize || subsampled || hphase || vphase || sun8i_vi_keep_scaler_enabled) {
> unsigned int scanline, required;
> struct drm_display_mode *mode;
> u32 hscale, vscale, fps;
> --
> 2.30.2
>
On Fri, Jun 03, 2022 at 11:57:35AM +0300, Roman Stratiienko wrote:
> Hi Maxime,
>
> пт, 3 июн. 2022 г. в 11:24, Maxime Ripard <[email protected]>:
> >
> > Hi,
> >
> > On Thu, Jun 02, 2022 at 06:01:18PM +0000, Roman Stratiienko wrote:
> > > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > > buffered, but de facto it doesn't, or the hardware has the shadow
> > > register latching issues which causes single-frame picture corruption
> > > after changing the state of scaler enable register.
> > >
> > > Allow the user to keep the scaler always enabled, preventing the UI
> > > glitches on the transition from scaled to unscaled state.
> > >
> > > NOTE:
> > > UI layer scaler has more registers with double-buffering issue and can't
> > > be workarounded in the same manner.
> > >
> > > You may find a python test and a demo video for this issue at [1]
> > >
> > > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
> >
> > Please describe the issue entirely here. The commit log must be self-sufficient.
>
> Commit message already states "single-frame picture corruption after
> changing the state of scaler enable register", therefore I find it
> already self-sufficient
>
> Also I find demo videos and link to tests useful for the followers to
> go further with investigation.
Until a couple of years, where that URL isn't valid anymore and it's just useless.
> If you have something specific in mind when asking to enhance the
> commit message please say it.
What sequence of events trigger the issue, what issue it triggers and
why your solution addresses it would be nice
> > > Signed-off-by: Roman Stratiienko <[email protected]>
> > > ---
> > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
> > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
> > > 2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > index 71ab0a00b4de..15cad0330f66 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > @@ -27,6 +27,18 @@
> > > #include "sun8i_vi_layer.h"
> > > #include "sunxi_engine.h"
> > >
> > > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > > + * register latching issues which causes single-frame picture corruption
> > > + * after changing the state of scaler enable register.
> > > + * Allow the user to keep the scaler always enabled, preventing the UI
> > > + * glitches on the transition from scaled to unscaled state.
> > > + */
> > > +int sun8i_vi_keep_scaler_enabled;
> > > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > > + "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
> > > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
> > > +
> >
> > It's not clear to me why we would want to make that a parameter?
> >
> >1 If it never works, we should fix it once and for all and not allow a broken setup at all.
>
> It's a hardware issue and can be fixed only within the hardware.
>
> Current patch is a workaround that if enabled can cause increased
> power consumption for existing users. Therefore I think it is better
> to give existing distro-maintainers a chance to test it prior to
> delivery.
Except distro-maintainers are likely never going to notice that flag
exists in the first place, won't be using it and thus the issue will
remain.
Maxime
Hi Ondrej,
пт, 3 июн. 2022 г. в 00:55, Ondřej Jirman <[email protected]>:
>
> Hi Roman,
>
> On Thu, Jun 02, 2022 at 06:01:18PM +0000, Roman Stratiienko wrote:
> > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > buffered, but de facto it doesn't, or the hardware has the shadow
> > register latching issues which causes single-frame picture corruption
> > after changing the state of scaler enable register.
> >
> > Allow the user to keep the scaler always enabled, preventing the UI
> > glitches on the transition from scaled to unscaled state.
> >
> > NOTE:
> > UI layer scaler has more registers with double-buffering issue and can't
> > be workarounded in the same manner.
> >
> > You may find a python test and a demo video for this issue at [1]
>
> Isn't this an issue with kernel driver not waiting for DE2 FINISH IRQ, but
> for VBLANK IRQ from TCON instead, before allowing to write new set of register
> values?
No, DE2 FINISH IRQ is triggered just some micro or even nanoseconds
earlier from vblank IRQ
(I have checked it using tracing).
I can guess MISSION FINISH IRQ triggered when the last line is being
sent to the line buffer cache (in between of the mixer output and tcon
input).
But VBLANG IRQ triggers when the last line is being sent to the
display + hfront porch time + hsync time.
I have also tried scheduling the register updates that have
double-buffering issues at VBLANK irq. And such a solution fixes the
test for both VI and UI scalers.
But under high loads there are still glitches happening. This patch
solves the issue for both test and high load conditions. (but for VI
only)
I'll post my solution with a deferred scaling register update soon.
>
> https://megous.com/dl/tmp/4fe35b3fc72ee7de.png
>
> I haven't checked if FINISH flag is set at time of VBLANK interrupt, so maybe
> this is not the issue.
>
> regards,
> o.
>
> > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
> > Signed-off-by: Roman Stratiienko <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
> > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index 71ab0a00b4de..15cad0330f66 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -27,6 +27,18 @@
> > #include "sun8i_vi_layer.h"
> > #include "sunxi_engine.h"
> >
> > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > + * register latching issues which causes single-frame picture corruption
> > + * after changing the state of scaler enable register.
> > + * Allow the user to keep the scaler always enabled, preventing the UI
> > + * glitches on the transition from scaled to unscaled state.
> > + */
> > +int sun8i_vi_keep_scaler_enabled;
> > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > + "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
> > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
> > +
> > struct de2_fmt_info {
> > u32 drm_fmt;
> > u32 de2_fmt;
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > index 662ba1018cc4..f005ab883503 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -17,6 +17,8 @@
> > #include "sun8i_vi_layer.h"
> > #include "sun8i_vi_scaler.h"
> >
> > +extern int sun8i_vi_keep_scaler_enabled;
> > +
> > static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
> > int overlay, bool enable, unsigned int zpos)
> > {
> > @@ -149,7 +151,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> > */
> > subsampled = format->hsub > 1 || format->vsub > 1;
> >
> > - if (insize != outsize || subsampled || hphase || vphase) {
> > + if (insize != outsize || subsampled || hphase || vphase || sun8i_vi_keep_scaler_enabled) {
> > unsigned int scanline, required;
> > struct drm_display_mode *mode;
> > u32 hscale, vscale, fps;
> > --
> > 2.30.2
> >
Hi Maxime,
пт, 3 июн. 2022 г. в 11:24, Maxime Ripard <[email protected]>:
>
> Hi,
>
> On Thu, Jun 02, 2022 at 06:01:18PM +0000, Roman Stratiienko wrote:
> > According to DE2.0/DE3.0 manual VI scaler enable register is double
> > buffered, but de facto it doesn't, or the hardware has the shadow
> > register latching issues which causes single-frame picture corruption
> > after changing the state of scaler enable register.
> >
> > Allow the user to keep the scaler always enabled, preventing the UI
> > glitches on the transition from scaled to unscaled state.
> >
> > NOTE:
> > UI layer scaler has more registers with double-buffering issue and can't
> > be workarounded in the same manner.
> >
> > You may find a python test and a demo video for this issue at [1]
> >
> > [1]: https://github.com/GloDroid/glodroid_tests/issues/4
>
> Please describe the issue entirely here. The commit log must be self-sufficient.
Commit message already states "single-frame picture corruption after
changing the state of scaler enable register", therefore I find it
already self-sufficient
Also I find demo videos and link to tests useful for the followers to
go further with investigation.
If you have something specific in mind when asking to enhance the
commit message please say it.
>
> > Signed-off-by: Roman Stratiienko <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
> > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index 71ab0a00b4de..15cad0330f66 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -27,6 +27,18 @@
> > #include "sun8i_vi_layer.h"
> > #include "sunxi_engine.h"
> >
> > +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> > + * buffered, but de facto it doesn't, or the hardware has the shadow
> > + * register latching issues which causes single-frame picture corruption
> > + * after changing the state of scaler enable register.
> > + * Allow the user to keep the scaler always enabled, preventing the UI
> > + * glitches on the transition from scaled to unscaled state.
> > + */
> > +int sun8i_vi_keep_scaler_enabled;
> > +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> > + "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
> > +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
> > +
>
> It's not clear to me why we would want to make that a parameter?
>
>1 If it never works, we should fix it once and for all and not allow a broken setup at all.
It's a hardware issue and can be fixed only within the hardware.
Current patch is a workaround that if enabled can cause increased
power consumption
for existing users. Therefore I think it is better to give existing
distro-maintainers a chance to test it prior to delivery.
Also I do not have all the sunxi SOCs to test it. My tests were
limited only by A64 and H6 SOCs.
> Maxime
Best regards,
Roman
Hi,
On Thu, Jun 02, 2022 at 06:01:18PM +0000, Roman Stratiienko wrote:
> According to DE2.0/DE3.0 manual VI scaler enable register is double
> buffered, but de facto it doesn't, or the hardware has the shadow
> register latching issues which causes single-frame picture corruption
> after changing the state of scaler enable register.
>
> Allow the user to keep the scaler always enabled, preventing the UI
> glitches on the transition from scaled to unscaled state.
>
> NOTE:
> UI layer scaler has more registers with double-buffering issue and can't
> be workarounded in the same manner.
>
> You may find a python test and a demo video for this issue at [1]
>
> [1]: https://github.com/GloDroid/glodroid_tests/issues/4
Please describe the issue entirely here. The commit log must be self-sufficient.
> Signed-off-by: Roman Stratiienko <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++++++++
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 4 +++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 71ab0a00b4de..15cad0330f66 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -27,6 +27,18 @@
> #include "sun8i_vi_layer.h"
> #include "sunxi_engine.h"
>
> +/* According to DE2.0/DE3.0 manual VI scaler enable register is double
> + * buffered, but de facto it doesn't, or the hardware has the shadow
> + * register latching issues which causes single-frame picture corruption
> + * after changing the state of scaler enable register.
> + * Allow the user to keep the scaler always enabled, preventing the UI
> + * glitches on the transition from scaled to unscaled state.
> + */
> +int sun8i_vi_keep_scaler_enabled;
> +MODULE_PARM_DESC(keep_vi_scaler_enabled,
> + "Keep VI scaler enabled (1 = enabled, 0 = disabled (default))");
> +module_param_named(keep_vi_scaler_enabled, sun8i_vi_keep_scaler_enabled, int, 0644);
> +
It's not clear to me why we would want to make that a parameter?
If it never works, we should fix it once and for all and not allow a broken setup at all.
Maxime
