[PATCH] drm/sun4i: Workaround TCON TOP conflict between DE0 and DE1

[Parthiban]

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