| Message ID | 20241108-tcon_fix-v1-1-616218cc0d5f@jookia.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/sun4i: Workaround TCON TOP conflict between DE0 and DE1 | expand |
On Fri, 08 Nov 2024 12:40:16 +1100 John Watts <contact@jookia.org> wrote: Hi John, thanks for taking care and sending a patch! > On the D1 and T113 the TCON TOP cannot handle setting both DEs to a > single output, even if the outputs are disabled. As a workaround assign > DE1 to TVE0 by default. Can you say *why* this patch is needed? Is there something broken that needs fixing? Where does this show and why wasn't this a problem before? > A full fix for this would include logic that makes sure both DEs never > share the same output. To be honest, given the isolation on this patch, I'd rather wait for this full fledged solution, especially if there is no pressing need (see above). > Signed-off-by: John Watts <contact@jookia.org> > --- > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > index a1ca3916f42bcc63b9ac7643e788d962ef360ca8..543311ffb1509face3fbfd069ded10933f254b9d 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > @@ -179,7 +179,7 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > * At least on H6, some registers have some bits set by default > * which may cause issues. Clear them here. > */ > - writel(0, regs + TCON_TOP_PORT_SEL_REG); > + writel(0x20, regs + TCON_TOP_PORT_SEL_REG); Sorry, but that looks weird: First, please explain the 0x20. Is it bit 5? If yes, what does that bit mean? The commit message suggests you know that? And second: the comment above clearly states that those two writes just *clear* some registers, to have some sane base line. So please adjust this comment, and copy in some of the rationale from the commit message. Explaining things in the commit message is good (so thanks for that!), but having at least some terse technical explanations near the code, in a comment, is better. Cheers, Andre > writel(0, regs + TCON_TOP_GATE_SRC_REG); > > /* > > --- > base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652 > change-id: 20241108-tcon_fix-f0585ac9bae0 > > Best regards,
On Fri, Nov 08, 2024 at 11:53:57AM +0000, Andre Przywara wrote: > Hi John, Hi Andre! > Can you say *why* this patch is needed? Is there something broken that > needs fixing? Where does this show and why wasn't this a problem before? Oops, that's a good point. There is currently a bug where the LCD output will be tinted. I have full context here which I should have probably linked in the patch description: https://lore.kernel.org/linux-sunxi/Zn8GVkpwXwhaUFno@titan/T/#u > To be honest, given the isolation on this patch, I'd rather wait for this > full fledged solution, especially if there is no pressing need (see above). I'd be interested to hear if that's still the wanted solution given the link above. This currently blocks many people from having working LCD output. Doing it the proper way might be overkill for now unless someone deliberately tries to run two DEs to the same output. I haven't seen this use case. Allwinner kernel fork initially sets them up to values like these then makes sure both DEs can't be set to the same TCON. > > - writel(0, regs + TCON_TOP_PORT_SEL_REG); > > + writel(0x20, regs + TCON_TOP_PORT_SEL_REG); > > Sorry, but that looks weird: > First, please explain the 0x20. Is it bit 5? If yes, what does that bit > mean? The commit message suggests you know that? > > And second: the comment above clearly states that those two writes just > *clear* some registers, to have some sane base line. So please adjust this > comment, and copy in some of the rationale from the commit message. > Explaining things in the commit message is good (so thanks for that!), but > having at least some terse technical explanations near the code, in a > comment, is better. Bit 5 is value 3 of TCON_TOP_PORT_DE1_MSK. The R40 datasheet explains the values of both masks as follows: 00: TCON_LCD0 01: TCON_LCD1 10: TCON_TV0 11: TCON_TV1 So this sets DE1's input to DE0. > > Cheers, > Andre Thanks, John Watts
On 11/8/24 6:59 PM, John Watts wrote: > On Fri, Nov 08, 2024 at 11:53:57AM +0000, Andre Przywara wrote: >> Hi John, > > Hi Andre! > >> Can you say *why* this patch is needed? Is there something broken that >> needs fixing? Where does this show and why wasn't this a problem before? > > Oops, that's a good point. There is currently a bug where the LCD output will > be tinted. I have full context here which I should have probably linked in the > patch description: > > https://lore.kernel.org/linux-sunxi/Zn8GVkpwXwhaUFno@titan/T/#u > >> To be honest, given the isolation on this patch, I'd rather wait for this >> full fledged solution, especially if there is no pressing need (see above). > > I'd be interested to hear if that's still the wanted solution given the link > above. This currently blocks many people from having working LCD output. > > Doing it the proper way might be overkill for now unless someone deliberately > tries to run two DEs to the same output. I haven't seen this use case. > > Allwinner kernel fork initially sets them up to values like these then makes > sure both DEs can't be set to the same TCON. > >>> - writel(0, regs + TCON_TOP_PORT_SEL_REG); >>> + writel(0x20, regs + TCON_TOP_PORT_SEL_REG); >> >> Sorry, but that looks weird: >> First, please explain the 0x20. Is it bit 5? If yes, what does that bit >> mean? The commit message suggests you know that? >> >> And second: the comment above clearly states that those two writes just >> *clear* some registers, to have some sane base line. So please adjust this >> comment, and copy in some of the rationale from the commit message. >> Explaining things in the commit message is good (so thanks for that!), but >> having at least some terse technical explanations near the code, in a >> comment, is better. > > Bit 5 is value 3 of TCON_TOP_PORT_DE1_MSK. The R40 datasheet explains the > values of both masks as follows: > > 00: TCON_LCD0 > 01: TCON_LCD1 > 10: TCON_TV0 > 11: TCON_TV1 > > So this sets DE1's input to DE0. To add, 0x20 will be DE0 <--> LCD0 and DE1 <--> TV0. Below note (copied from R40) states the priority of the DE selection, which fails to work? Not sure, may be disabling CORE1_SCLK_GATE and CORE1_HCLK_GATE in de2-clk helps. With A133 following the same as T113 with single mixer without TV, still sets 0x20 in vendor kernel. copied from R40: Note: The priority of DE0 is higher than DE1. If TCON_LCD0 selects DE0 and DE1 as source at the same time, then DE0 will be used for the source of TCON_LCD0. Thanks, Parthiban > >> >> Cheers, >> Andre > > Thanks, > John Watts >
On Fri, Nov 08, 2024 at 07:36:16PM +0530, Parthiban wrote: > To add, 0x20 will be DE0 <--> LCD0 and DE1 <--> TV0. Below note (copied from > R40) states the priority of the DE selection, which fails to work? Not sure, > may be disabling CORE1_SCLK_GATE and CORE1_HCLK_GATE in de2-clk helps. > > With A133 following the same as T113 with single mixer without TV, still > sets 0x20 in vendor kernel. > > copied from R40: > Note: The priority of DE0 is higher than DE1. > If TCON_LCD0 selects DE0 and DE1 as source at the same time, then > DE0 will be used for the source of TCON_LCD0. Hi there, Yes that was a pretty bad typo, I meant to say DE1 to TV0 The prioritization seems broken in the T113 at least, it's racy from what I see in testing. I should note this in the patch too. I looked at the datasheets and kernel code briefly: I can't seem to figure out what SCLK/HCLK gating does and I don't think the kernel touches these registers which are gated by default. > Thanks, > Parthiban John Watts
Hey everyone, I'm not sure exactly where to add this but I discussed some of this with Parthiban on #linux-sunxi a few days ago, so I want to write it down before I work on the next version of the patch. I had assumed for some reason in my mind that DE0 and DE1 here referred to mixers, but they actually refer to chips that have multiple DEs. It looks like at least with the A133 it has two DEs instead of two mixers. This can be found by looking at the Allwinner BSP: SUN50IW10 requires CONFIG_INDEPENDENT_DE and has a device tree with an extra reg and clock: <0x0 0x06800000 0x0 0x3fffff>,/*de1*/ <&clk_dpss_top1> However the tcon-top code seems to conflate mixers and DE in the mainline code and the Allwinner code. So ... It seems like 'DE0' and 'DE1' really do mean mixers in this case. It's probably best to note that down. I thought a bit more about how to solve this properly- setting two mixers to the same output is something people probably won't do in practice, so the only way you could really arrive at this bugged state is by setting it as the default state. This patch may be the correct solution after all. John Watts On Sat, Nov 09, 2024 at 01:15:16AM +1100, John Watts wrote: > On Fri, Nov 08, 2024 at 07:36:16PM +0530, Parthiban wrote: > > To add, 0x20 will be DE0 <--> LCD0 and DE1 <--> TV0. Below note (copied from > > R40) states the priority of the DE selection, which fails to work? Not sure, > > may be disabling CORE1_SCLK_GATE and CORE1_HCLK_GATE in de2-clk helps. > > > > With A133 following the same as T113 with single mixer without TV, still > > sets 0x20 in vendor kernel. > > > > copied from R40: > > Note: The priority of DE0 is higher than DE1. > > If TCON_LCD0 selects DE0 and DE1 as source at the same time, then > > DE0 will be used for the source of TCON_LCD0. > > Hi there, > > Yes that was a pretty bad typo, I meant to say DE1 to TV0 > The prioritization seems broken in the T113 at least, it's racy from > what I see in testing. I should note this in the patch too. > > I looked at the datasheets and kernel code briefly: I can't seem to > figure out what SCLK/HCLK gating does and I don't think the kernel > touches these registers which are gated by default. > > > Thanks, > > Parthiban > > John Watts
On 11/12/24 5:27 PM, John Watts wrote: > Hey everyone, > > I'm not sure exactly where to add this but I discussed some of this with > Parthiban on #linux-sunxi a few days ago, so I want to write it down > before I work on the next version of the patch. > > I had assumed for some reason in my mind that DE0 and DE1 here referred > to mixers, but they actually refer to chips that have multiple DEs. It > looks like at least with the A133 it has two DEs instead of two mixers. > > This can be found by looking at the Allwinner BSP: SUN50IW10 requires > CONFIG_INDEPENDENT_DE and has a device tree with an extra reg and clock: > > <0x0 0x06800000 0x0 0x3fffff>,/*de1*/ > <&clk_dpss_top1> Independent DE is unique to A133/A100 AFAIK. > > However the tcon-top code seems to conflate mixers and DE in the > mainline code and the Allwinner code. So ... It seems like 'DE0' and > 'DE1' really do mean mixers in this case. It's probably best to note #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. Handling for mixer0 <-> lcd1 and mixer1 <-> lcd0 also needs to set DE2TCON_MUX in de clock, which is missing now. > that down. > > I thought a bit more about how to solve this properly- setting two > mixers to the same output is something people probably won't do in > practice, so the only way you could really arrive at this bugged state > is by setting it as the default state. This patch may be the correct > solution after all. 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? Thanks, Parthiban > > John Watts > > On Sat, Nov 09, 2024 at 01:15:16AM +1100, John Watts wrote: >> On Fri, Nov 08, 2024 at 07:36:16PM +0530, Parthiban wrote: >>> To add, 0x20 will be DE0 <--> LCD0 and DE1 <--> TV0. Below note (copied from >>> R40) states the priority of the DE selection, which fails to work? Not sure, >>> may be disabling CORE1_SCLK_GATE and CORE1_HCLK_GATE in de2-clk helps. >>> >>> With A133 following the same as T113 with single mixer without TV, still >>> sets 0x20 in vendor kernel. >>> >>> copied from R40: >>> Note: The priority of DE0 is higher than DE1. >>> If TCON_LCD0 selects DE0 and DE1 as source at the same time, then >>> DE0 will be used for the source of TCON_LCD0. >> >> Hi there, >> >> Yes that was a pretty bad typo, I meant to say DE1 to TV0 >> The prioritization seems broken in the T113 at least, it's racy from >> what I see in testing. I should note this in the patch too. >> >> I looked at the datasheets and kernel code briefly: I can't seem to >> figure out what SCLK/HCLK gating does and I don't think the kernel >> touches these registers which are gated by default. >> >>> Thanks, >>> Parthiban >> >> John Watts
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
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
Hi, On Wed, Nov 13, 2024 at 11:40:14AM +0530, Parthiban wrote: > 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. After an hour or two of spelunking the code base, I'm still not sure about this. Confusables: - sun8i_tcon_top_de_config uses 'mixer' instead of DE - The datasheet for TCON TOP mentions DEs - The DT documentation for TCON TOP only mentions mixers - Only mixers are passed to the TCON TOP DT - Allwinner's tcon_de_attach works on systems without two DEs - Allwinner's tcon_de_attach special cases systems with two DEs - Allwinner's tcon_de_attach implies theres two TCON TOPs for two DEs However sun8i_r40_tcon_tv_set_mux seems to clear this up: - The register value is found using the remote endpoint reg - tcon_top_mixer0_out specifies 0 and 2 for lcd0 and tv0 - tcon_top_mixer1_out also specifies 0 and 2 for lcd0 and tv0 - The 'mixer' is engine->id - engine->id is found using sun4i_tcon_find_engine - It gets the id from the endpoint number - mixer0 has id 0 - mixer0 has id 1 - there's one engine struct per mixer and display-backend It really seems like the code means mixers here. > > 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. So if Allwinner and mainline don't use it, what does? > 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. If my thoughts are correct, this would break use of mixer0 and mixer1 at the same time. > 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, John Watts.
diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c index a1ca3916f42bcc63b9ac7643e788d962ef360ca8..543311ffb1509face3fbfd069ded10933f254b9d 100644 --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c @@ -179,7 +179,7 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, * At least on H6, some registers have some bits set by default * which may cause issues. Clear them here. */ - writel(0, regs + TCON_TOP_PORT_SEL_REG); + writel(0x20, regs + TCON_TOP_PORT_SEL_REG); writel(0, regs + TCON_TOP_GATE_SRC_REG); /*
On the D1 and T113 the TCON TOP cannot handle setting both DEs to a single output, even if the outputs are disabled. As a workaround assign DE1 to TVE0 by default. A full fix for this would include logic that makes sure both DEs never share the same output. Signed-off-by: John Watts <contact@jookia.org> --- drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652 change-id: 20241108-tcon_fix-f0585ac9bae0 Best regards,