On 11/13/24 5:03 AM, John Watts wrote: > 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 No, Mixers in upstream refers to RT-Mixers inside the DE. It's only the quirk for R40/D1 setting the DE ports using mixer numbering. > 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. Gave a quick try, but display went blue. My current assumption is that it's called INDEPENDENT DE, so DE1 <-> LCD1 is the only possibility. Yet to try that pipeline. > >> 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? Missing in the upstream. > > 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? :) So far there is no real user for DE1 in upstream. DE_PORT_SELECT_REG value for DE1 can be negate of DE0, so that they won't conflict or cause timing issues. Also DE_PORT_SELECT_REG mentions only about TV and LCD muxing, but missing HDMI, DSI and so. Otherwise, if I get DE1 working in A133, I will try to add quirk to set DE0 and DE1 port mapping in that case to respective connector. Thanks, Parthiban > >> Thanks, >> Parthiban > > Thanks for your input! > > John Watts
