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

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

[Wen He]

When we running DDR benchmark test will able to observed flicker issue
in 4k@60 resolution on monitor. In LS1028A SoC, need increasing DP500
priority to avoid that issue.

Signed-off-by: Wen He <wen.he_1@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)
+		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);
+#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
+#endif
 
 /* register offsets and bits specific to DP550/DP650 */
 #define MALIDP550_ADDR_SPACE_SIZE	0x10000
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[liviu.dudau]

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@60 resolution on monitor. In LS1028A SoC, need increasing DP500
> priority to avoid that issue.
> 
> Signed-off-by: Wen He <wen.he_1@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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Wen He]

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 2019年3月29日 0:39
> To: Wen He <wen.he_1@nxp.com>
> Cc: brian.starkey@arm.com; malidp@foss.arm.com;
> dri-devel@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@60 resolution on monitor. In LS1028A SoC, need increasing DP500
> > priority to avoid that issue.
> >
> > Signed-off-by: Wen He <wen.he_1@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@60, 720p@60, 1080@60, 4k@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!  /
>   ---------------
>     ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[liviu.dudau]

On Fri, Mar 29, 2019 at 08:20:00AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 2019年3月29日 0:39
> > To: Wen He <wen.he_1@nxp.com>
> > Cc: brian.starkey@arm.com; malidp@foss.arm.com;
> > dri-devel@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@60 resolution on monitor. In LS1028A SoC, need increasing DP500
> > > priority to avoid that issue.
> > >
> > > Signed-off-by: Wen He <wen.he_1@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,

Hi Wen,

> 
> Let me clearly it.
> Currently, we only supported four resolutions (480p@60, 720p@60, 1080@60, 4k@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.

Yes, but you are asking to merge this into the mainline driver which
supports more resolutions than that.

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

If you want to do that for your BSP, then it's fine. For mainline, I
think we can do better and think in more generic terms.

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

Bit 1 in MALIDP500_CONFIG_VALID is not defined in the Arm spec, so don't
know what the NXP design team has done there, might be worth talking to
them.

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

Yes, before we enter modeset we set bit 0 of MALIDP500_CONFIG_VALID and we clear
it after modeset is finished. DP500 is a bit weird, you need to look at
CONFIG_VALID as somewhat inverted as a signal from the software point of view.
It is probably easier to think of it in the way the code was written,
i.e. we "enter config mode" before modeset and then "exit config mode"
afterwards.

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

What I was thinking was that you could add the properties and the
documentation ;)

Best regards,
Liviu

> 
> 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!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Wen He]

> -----Original Message-----
> From: liviu.dudau@arm.com <liviu.dudau@arm.com>
> Sent: 2019年3月30日 0:11
> To: Wen He <wen.he_1@nxp.com>
> Cc: brian.starkey@arm.com; malidp@foss.arm.com;
> dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/arm/malidp: Setting QoS priority for 4k to avoid
> flicker issue
> 
> On Fri, Mar 29, 2019 at 08:20:00AM +0000, Wen He wrote:
> >
> >
> > > -----Original Message-----
> > > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > > Sent: 2019年3月29日 0:39
> > > To: Wen He <wen.he_1@nxp.com>
> > > Cc: brian.starkey@arm.com; malidp@foss.arm.com;
> > > dri-devel@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@60 resolution on monitor. In LS1028A SoC, need
> > > > increasing DP500 priority to avoid that issue.
> > > >
> > > > Signed-off-by: Wen He <wen.he_1@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,
> 
> Hi Wen,
> 
> >
> > Let me clearly it.
> > Currently, we only supported four resolutions (480p@60, 720p@60,
> 1080@60, 4k@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.
> 
> Yes, but you are asking to merge this into the mainline driver which supports
> more resolutions than that.
> 
> >
> > > 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.
> 
> If you want to do that for your BSP, then it's fine. For mainline, I think we can
> do better and think in more generic terms.
> 
> >
> > > 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.
> 
> Bit 1 in MALIDP500_CONFIG_VALID is not defined in the Arm spec, so don't
> know what the NXP design team has done there, might be worth talking to
> them.
> 
> >
> > Is there can always to valid configuration? If yes, I can remove this line.
> 
> Yes, before we enter modeset we set bit 0 of MALIDP500_CONFIG_VALID and
> we clear it after modeset is finished. DP500 is a bit weird, you need to look at
> CONFIG_VALID as somewhat inverted as a signal from the software point of
> view.
> It is probably easier to think of it in the way the code was written, i.e. we
> "enter config mode" before modeset and then "exit config mode"
> afterwards.

Hi Liviu,

This patch is not a rigorous implementation for the mainline, I will use the new
Implementation to send next version of the patch. 

Thanks for these comments.

Best Regards,
Wen

> 
> >
> > >
> > > > +#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.
> 
> What I was thinking was that you could add the properties and the
> documentation ;)
> 
> Best regards,
> Liviu
> 
> >
> > 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!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel