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

[Wen He]

> -----Original Message-----
> From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
> Sent: 2019年3月29日 0:39
> To: Wen He <wen.he_1 at nxp.com>
> Cc: brian.starkey at arm.com; malidp at foss.arm.com;
> dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/arm/malidp: Setting QoS priority for 4k to avoid
> flicker issue
> 
> 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?
> 

Hi liviu,

Let me clearly it.
Currently, we only supported four resolutions (480p at 60, 720p at 60, 1080 at 60, 4k at 60) for LS1028A Display port.
All of the refresh rate is 60MHz. so that impossible will be there resolution output's 2160x3840 and refresh rate is 120MHz.

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

Yes, you are right. 
But only 4k resolution have the flicker issue, so this is a special problem. 
I think that hardcode is better than dynamically adjusting bandwidth.

> 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.
> 

About this, I got this step from our design team, So just to make sure the configuration works that 
writing value 0x3 to MALIDP500_CONFIG_VALID.

Is there can always to valid configuration? If yes, I can remove this line.

> 
> > +#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
> 

These values are QoS registers of DP500.
I have already put CONFIG_ARCH_LAYERSCAPE definition to specify architecture.

Regarding your suggestion, I think device tree also can meet this, but that is 
Non-standard writing in here, because not have any properties define to describe QoS 
in arm,malidp device bindings file.

Best Regards,
Wen
> 
> > +#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!  /
>   ---------------
>     ¯\_(ツ)_/¯