Re: [RFC 1/4] drm/sun4i: Wait for previous mixing process finish before committing next

Jernej Škrabec
2019-12-29T09:11:46Z

Hi!

Dne sobota, 28. december 2019 ob 21:28:15 CET je
roman.stratiienko@globallogic.com napisal(a):
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
>
> Screen composition that requires dynamic layout modification,
> especially scaling is corrupted when layout changes.
>
> For example if one of the layer scales down, misaligned lines can be
> observed, and dynamic increasing of destination area makes mixer to hang
> and draw nothing after drawing modified layer.
>
> After deep investigation it turns that scaler double-buffered registers
> are not latched by GLB_DBUFFER bit, instead thay are latched immidiately.
>
> Only way to avoid artifacts is to change the registers after mixer finish
> previous frame.
>
> Similar was made in sunxi BSP - scaler register values was stored into RAM,
> and moved into registers at sync together with GLB_DBUFFER.
>
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>

Nice catch! However, I'm a bit worried about blocking nature of this solution.
What about shadowing scaler registers and applying them in "finish_irq"
handler? You see, VI scaler can in some cases consume almost all time between
two VSync events. That issue came up on A64 mixer1 with downscaling 4K videos
in real time. I imagine that this solution might block for too long in this
case.

Check VI coarse scaling code:
https://elixir.bootlin.com/linux/v5.5-rc3/source/drivers/gpu/drm/sun4i/
sun8i_vi_layer.c#L144

> ---
> drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++++++
> drivers/gpu/drm/sun4i/sun8i_mixer.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 8b803eb903b8..eea4813602b7
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -257,6 +257,20 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> format) return NULL;
> }
>
> +static void sun8i_atomic_begin(struct sunxi_engine *engine,
> + struct drm_crtc_state *old_state)
> +{
> + int reg, ret;
> +
> + ret = regmap_read_poll_timeout(engine->regs,
SUN8I_MIXER_GLOBAL_STATUS,
> + reg,
> + !(reg &
SUN8I_MIXER_GLOBAL_STATUS_BUSY),
> + 200, 100000);
> +
> + if (ret)
> + pr_warn("%s: Wait for frame finish timeout\n", __func__);

Newly introduced drm_warn() should be used here.

Best regards,
Jernej

> +}
> +
> static void sun8i_mixer_commit(struct sunxi_engine *engine)
> {
> DRM_DEBUG_DRIVER("Committing changes\n");
> @@ -310,6 +324,7 @@ static struct drm_plane **sun8i_layers_init(struct
> drm_device *drm, static const struct sunxi_engine_ops sun8i_engine_ops = {
> .commit = sun8i_mixer_commit,
> .layers_init = sun8i_layers_init,
> + .atomic_begin = sun8i_atomic_begin,
> };
>
> static struct regmap_config sun8i_mixer_regmap_config = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index c6cc94057faf..915479cc3077
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -25,6 +25,8 @@
>
> #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE BIT(0)
>
> +#define SUN8I_MIXER_GLOBAL_STATUS_BUSY BIT(4)
> +
> #define DE2_MIXER_UNIT_SIZE 0x6000
> #define DE3_MIXER_UNIT_SIZE 0x3000