lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZzPluoI7xSTwhNcm@titan>
Date: Wed, 13 Nov 2024 10:33:14 +1100
From: John Watts <contact@...kia.org>
To: Parthiban <parthiban@...umiz.com>
Cc: Andre Przywara <andre.przywara@....com>,
	Maxime Ripard <mripard@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Samuel Holland <samuel@...lland.org>,
	dri-devel@...ts.freedesktop.org,
	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/sun4i: Workaround TCON TOP conflict between DE0 and
 DE1

Hi there,

On Tue, Nov 12, 2024 at 10:43:44PM +0530, Parthiban wrote:
> #define TCON_TOP_PORT_DE0_MSK                   GENMASK(1, 0)
> #define TCON_TOP_PORT_DE1_MSK                   GENMASK(5, 4)
> 
> references towards DE0 and DE1 is for DE itself, not the mixers in the
> current implementation.

So the datasheet says it's for DE0/DE1 but the Allwinner driver and mainline
driver assume it's for mixers. There's a conflation between mixer and DE in
this case, especially because everywhere mixer1 is used on the A133 it is
switched to DE1. I'm also unaware of the R40 having two DEs which kind of
confirms this might be a typo. If anyone has actually tested the second output
of this it would help find out if it actually means DE1 or mixer1.

> Handling for mixer0 <-> lcd1 and mixer1 <-> lcd0 also needs to set
> DE2TCON_MUX in de clock, which is missing now.

Hmm. Are you sure? Looking at the Allwinner drivers it has the method
de_top_set_de2tcon_mux in
drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_v33x/de330/de_top.c
which I think means it's for DE3? But I don't see it called anywhere?

This might be worth discussing in the DE3 patchset.

> sun8i_tcon_top_set_hdmi_src for R40 already sets these values via quirks.
> i.e controlling the port muxing. Also D1 quirks is same as R40. So the
> original changes to make the DE1 point to TVx can also done in this quirk
> without hardcoded value?

In this case I'm using an LCD which isn't HDMI, so I'm not too sure how much
this would help. Having it as a quirk also seems a bit overkill if this is a
general preventative fix, especially since Allwinner doesn't seem to test their
functionality. Relying on it seems like a mistake in this case.

My other thought is that when sun8i_tcon_top_de_config is called it could do
something. But I'm not sure what that something would actually be, given it may
be called twice in an (I assume) unknown order.

Say, if mixer1 is set as TV0 and and mixer0 is set as TV1 we would try to set mixer1
first, see that mixer0 is already set to TV0 then ... error? Even though the
final configuration doesn't have any conflicts.

I was thinking something like this for my next patch:

	/*
	 * Make sure that by default DE0 and DE1 are set to different outputs,
	 * otherwise we get a strange tinting or unusable display on the T113.
	*/
	reg = readl(regs + TCON_TOP_PORT_SEL_REG);
	reg &= ~TCON_TOP_PORT_DE0_MSK;
	reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, 0);
	reg &= ~TCON_TOP_PORT_DE1_MSK;
	reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, 1);
	writel(reg, regs + TCON_TOP_PORT_SEL_REG);

Perhaps this could be hidden behind a quirk? I would have to check to see which
chips have this behaviour, I'm not sure if it's a bug specific to the T113 or
D1/T113 or R40 too.

Also noting at the top of the file that DE0 and DE1 mean mixer0 and mixer1
might be good to reduce confusion.

What do you think? :)

> Thanks,
> Parthiban

Thanks for your input!

John Watts

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ