The DE mixer clock is currently set to run at 150 MHz, while the
Allwinner BSP configures it at 300 MHz and other platforms typically
run at 297 MHz.

150 MHz appears to be enough given the restricted graphics capabilities
of the SoC (with a work area of only 1024x1024). However it typically
causes the DE clock to be parented to the periph0 pll instead of the
video PLL.

While this should generally not be a concern, it appears (based on
experimentation) that both the DE and TCON clocks need to be parented
to the same PLL for these units to work. While we cannot represent this
constraint in the clock driver, it appears that the TCON clock will
often get parented to the video pll (typically running at 297 MHz for
the CSI units needs), for instance when driving displays with a 33 MHz
pixel clock (33 being a natural divider of 297).

Running the DE clock at 297 MHz will typically result in parenting to
the video pll instead of the periph0 pll, thus making the display
output functional.

This is all a bit fragile but it solves the issue with displays running
at 33 Mhz and brings V3s to use the same frequency as other platforms,
making support more unified.

Also align and sort the fields of the v3s mixer config while at it.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 8b41d33baa30..35fdc2451060 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -670,12 +670,12 @@ static const struct sun8i_mixer_cfg sun8i_r40_mixer1_cfg = {
 };
 
 static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = {
-	.vi_num = 2,
-	.ui_num = 1,
-	.scaler_mask = 0x3,
-	.scanline_yuv = 2048,
-	.ccsc = CCSC_MIXER0_LAYOUT,
-	.mod_rate = 150000000,
+	.ccsc		= CCSC_MIXER0_LAYOUT,
+	.mod_rate	= 297000000,
+	.scaler_mask	= 0x3,
+	.scanline_yuv	= 2048,
+	.ui_num		= 1,
+	.vi_num		= 2,
 };
 
 static const struct sun8i_mixer_cfg sun20i_d1_mixer0_cfg = {
-- 
2.49.0

On Tue, Jul 01, 2025 at 10:11:24PM +0200, Paul Kocialkowski wrote:
> The DE mixer clock is currently set to run at 150 MHz, while the
> Allwinner BSP configures it at 300 MHz and other platforms typically
> run at 297 MHz.
> 
> 150 MHz appears to be enough given the restricted graphics capabilities
> of the SoC (with a work area of only 1024x1024). However it typically
> causes the DE clock to be parented to the periph0 pll instead of the
> video PLL.
> 
> While this should generally not be a concern, it appears (based on
> experimentation) that both the DE and TCON clocks need to be parented
> to the same PLL for these units to work. While we cannot represent this
> constraint in the clock driver, it appears that the TCON clock will
> often get parented to the video pll (typically running at 297 MHz for
> the CSI units needs), for instance when driving displays with a 33 MHz
> pixel clock (33 being a natural divider of 297).
> 
> Running the DE clock at 297 MHz will typically result in parenting to
> the video pll instead of the periph0 pll, thus making the display
> output functional.
> 
> This is all a bit fragile but it solves the issue with displays running
> at 33 Mhz and brings V3s to use the same frequency as other platforms,
> making support more unified.

It's beyond fragile, and doesn't have anything to do with the DRM driver.

You should set up the clock tree properly in the clock driver, and then
add NO_REPARENT to the DE clock to make sure it stays that way.

And then, you can change the clock rate if you want to, but at least you
don't set a rate and hope that the side effects work your way, and won't
happen again.

Maxime

Hi Maxime,

Le Wed 02 Jul 25, 13:36, Maxime Ripard a écrit :
> On Tue, Jul 01, 2025 at 10:11:24PM +0200, Paul Kocialkowski wrote:
> > The DE mixer clock is currently set to run at 150 MHz, while the
> > Allwinner BSP configures it at 300 MHz and other platforms typically
> > run at 297 MHz.
> > 
> > 150 MHz appears to be enough given the restricted graphics capabilities
> > of the SoC (with a work area of only 1024x1024). However it typically
> > causes the DE clock to be parented to the periph0 pll instead of the
> > video PLL.
> > 
> > While this should generally not be a concern, it appears (based on
> > experimentation) that both the DE and TCON clocks need to be parented
> > to the same PLL for these units to work. While we cannot represent this
> > constraint in the clock driver, it appears that the TCON clock will
> > often get parented to the video pll (typically running at 297 MHz for
> > the CSI units needs), for instance when driving displays with a 33 MHz
> > pixel clock (33 being a natural divider of 297).
> > 
> > Running the DE clock at 297 MHz will typically result in parenting to
> > the video pll instead of the periph0 pll, thus making the display
> > output functional.
> > 
> > This is all a bit fragile but it solves the issue with displays running
> > at 33 Mhz and brings V3s to use the same frequency as other platforms,
> > making support more unified.
> 
> It's beyond fragile, and doesn't have anything to do with the DRM driver.
> 
> You should set up the clock tree properly in the clock driver, and then
> add NO_REPARENT to the DE clock to make sure it stays that way.

Thanks for the suggestion! I wasn't aware there was a flag to avoid
reparenting, sounds like the most reasonable way to solve this issue then.

I'll send another iteration reworking the clock tree then.

> And then, you can change the clock rate if you want to, but at least you
> don't set a rate and hope that the side effects work your way, and won't
> happen again.

We might as well still change it. To be honest I don't really see why it was
set to 150 MHz in the first place.

Cheers,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

在 2025-07-01星期二的 22:11 +0200，Paul Kocialkowski写道：
> The DE mixer clock is currently set to run at 150 MHz, while the
> Allwinner BSP configures it at 300 MHz and other platforms typically
> run at 297 MHz.
> 
> 150 MHz appears to be enough given the restricted graphics
> capabilities
> of the SoC (with a work area of only 1024x1024). However it typically
> causes the DE clock to be parented to the periph0 pll instead of the
> video PLL.
> 
> While this should generally not be a concern, it appears (based on
> experimentation) that both the DE and TCON clocks need to be parented
> to the same PLL for these units to work. While we cannot represent
> this
> constraint in the clock driver, it appears that the TCON clock will
> often get parented to the video pll (typically running at 297 MHz for

Ah I think pixel clocks are arbitary and nothing could be predicted
about it...

> the CSI units needs), for instance when driving displays with a 33
> MHz
> pixel clock (33 being a natural divider of 297).
> 
> Running the DE clock at 297 MHz will typically result in parenting to
> the video pll instead of the periph0 pll, thus making the display
> output functional.
> 
> This is all a bit fragile but it solves the issue with displays
> running
> at 33 Mhz and brings V3s to use the same frequency as other
> platforms,
> making support more unified.
> 
> Also align and sort the fields of the v3s mixer config while at it.
> 
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 8b41d33baa30..35fdc2451060 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -670,12 +670,12 @@ static const struct sun8i_mixer_cfg
> sun8i_r40_mixer1_cfg = {
>  };
>  
>  static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = {
> -       .vi_num = 2,
> -       .ui_num = 1,
> -       .scaler_mask = 0x3,
> -       .scanline_yuv = 2048,
> -       .ccsc = CCSC_MIXER0_LAYOUT,
> -       .mod_rate = 150000000,
> +       .ccsc           = CCSC_MIXER0_LAYOUT,
> +       .mod_rate       = 297000000,
> +       .scaler_mask    = 0x3,
> +       .scanline_yuv   = 2048,
> +       .ui_num         = 1,
> +       .vi_num         = 2,
>  };
>  
>  static const struct sun8i_mixer_cfg sun20i_d1_mixer0_cfg = {