[PATCH 2/2] drm/arm/malidp: Setting QoS priority for 4k to avoid flicker issue

[liviu.dudau at arm.com]

Hi Wen,

On Thu, Mar 28, 2019 at 03:56:58AM +0000, Wen He wrote:
> When we running DDR benchmark test will able to observed flicker issue
> in 4k at 60 resolution on monitor. In LS1028A SoC, need increasing DP500
> priority to avoid that issue.
> 
> Signed-off-by: Wen He <wen.he_1 at nxp.com>
> ---
>  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
>  drivers/gpu/drm/arm/malidp_regs.h |  8 ++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 8df12e9a33bb..a5263488eb02 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -378,6 +378,19 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
>  		malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
>  	else
>  		malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
> +
> +#ifdef CONFIG_ARCH_LAYERSCAPE
> +	/* Setting QoS for 4k resolution to avoid flicker issue */
> +	if (mode->hactive == 3840
> +			&& mode->vactive == 2160)

I'm not keen on this check, it only enables this for one 3840x2160.
What if the output is 2160x3840? Does it matter what pixel clock you
use? Can the same issue be seen if (for example) you use 120Hz refresh
rate but on a smaller output resolution?

I think it's worth thinking this in terms of bandwidth and not hardcode
for one resolution.

Also, I think setting the QoS bits should be a bit more generic, i.e if
the modeset requires a certain bandwidth you should write a value read
from a device tree, for example?

> +		malidp_hw_setbits(hwdev, GREEN_ARQOS_CONFIG
> +				| RED_ARQOS_CONFIG, MALIDP500_RQOS_QUALITY);
> +	else
> +		malidp_hw_clearbits(hwdev, GREEN_ARQOS_CONFIG
> +				| RED_ARQOS_CONFIG, MALIDP500_RQOS_QUALITY);
> +
> +	malidp_hw_setbits(hwdev, CONFIG_VALID, MALIDP500_CONFIG_VALID);

malidp500_modeset() will be called with MALIDP_CFG_VALID set anyway, so
you should not need this. I don't know what bit 1 in MALIDP500_CONFIG_VALID does
for LS1028A, I don't have that spec.


> +#endif
>  }
>  
>  int malidp_format_get_bpp(u32 fmt)
> diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> index a0dd6e1676a8..8dcf7e9f46ce 100644
> --- a/drivers/gpu/drm/arm/malidp_regs.h
> +++ b/drivers/gpu/drm/arm/malidp_regs.h
> @@ -213,6 +213,14 @@
>  #define MALIDP500_DC_IRQ_BASE		0x00f00
>  #define MALIDP500_CONFIG_VALID		0x00f00
>  #define MALIDP500_CONFIG_ID		0x00fd4
> +#ifdef CONFIG_ARCH_LAYERSCAPE
> +#define MALIDP500_RQOS_QUALITY          0x00500
> +/* Increasing QoS level signal */
> +#define GREEN_ARQOS_CONFIG              (0xd << 28)
> +/* Close to underflow QoS level signal */
> +#define RED_ARQOS_CONFIG                (0xd << 12)
> +#define CONFIG_VALID                    0x3

What are these values? Is it something NXP has added to the DP500? If so, I
think these should have some LAYERSCAPE (or LS) in their name. Also,
rather than hardcoding the values, would it not be better to read them
form device tree, to allow for fine tuning or variability between IP
deployments?

Best regards,
Liviu


> +#endif
>  
>  /* register offsets and bits specific to DP550/DP650 */
>  #define MALIDP550_ADDR_SPACE_SIZE	0x10000
> -- 
> 2.17.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯