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

[Jernej Åkrabec]

Hi!

Dne sobota, 28. december 2019 ob 21:28:15 CET je 
roman.stratiienko@xxxxxxxxxxxxxxx napisal(a):
> From: Roman Stratiienko <roman.stratiienko@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>

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