On 3/8/19 2:12 PM, Jacopo Mondi wrote: > Hi Laurent, > > On Fri, Mar 08, 2019 at 01:29:38PM +0200, Laurent Pinchart wrote: >> Hi Jacopo, >> >> On Thu, Mar 07, 2019 at 11:35:11AM +0100, Jacopo Mondi wrote: >>> On Wed, Mar 06, 2019 at 09:15:21PM +0200, Laurent Pinchart wrote: >>>> On Wed, Mar 06, 2019 at 12:26:59PM +0100, Jacopo Mondi wrote: >>>>> When both the media links between AFE and HDMI and the two TX CSI-2 outputs >>>>> gets disabled, the routing register ADV748X_IO_10 gets zeroed causing both >>>>> TXA and TXB output to get disabled. >>>>> >>>>> This causes some HDMI transmitters to stop working after both AFE and >>>>> HDMI links are disabled. >>>> >>>> Could you elaborate on why this would be the case ? By HDMI transmitter, >>>> I assume you mean the device connected to the HDMI input of the ADV748x. >>>> Why makes it fail (and how ?) when the TXA and TXB are both disabled ? >>> >>> I know, it's weird, the HDMI transmitter is connected to the HDMI >>> input of adv748x and should not be bothered by CSI-2 outputs >>> enablement/disablement. >>> >>> BUT, when I developed the initial adv748x AFE->TXA patches I was >>> testing HDMI capture using a laptop, and things were smooth. >>> >>> I recently started using a chrome cast device I found in some drawer >>> to test HDMI, as with it I don't need to go through xrandr as I had to >>> do when using a laptop for testing, but it seems the two behaves differently. >>> >>> Failures are of different types: from detecting a non-realisting >>> resolution from the HDMI subdevice, and then messing up the pipeline >>> configuration, to capture operations apparently completing properly >>> but resulting in mangled images. >>> >>> Do not deactivate the CSI-2 ouputs seems to fix the issue for the >>> Chromecast, and still work when capturing from laptop. There might be >>> something I am missing about HDMI maybe, but the patch not just fixes >>> the issue for me, but it might make sense on its own as disabling the >>> TXes might trigger some internal power saving state, or simply mess up >>> the HDMI link. >> >> I think this needs more investigation. It feels to me that you're >> working around an issue by chance, and it will come back to bite us >> later :-( >> > > I'm sorry I really can't tell what's happening, and why it is > happening on that specific device, which I cannot debug for sure. > > Ian suggested a possible cause, but I cannot tell due to my > HDMI-ignorance. I agree with Ian that it is likely related to EDID and/or HPD handling of the adv748x. The only other option is if the HDMI transmitter supports RxSense (i.e. detecting the pull-ups of the TMDS clock lines as a way of detecting that the transmitter is connected to a display). Not many transmitters support RxSense, though. HPD and/or EDID are the most likely culprits. It certainly has nothing to do with the CSI ports as such. Regards, Hans > >>> As disabling both TXes usually happens at media link reset time, just >>> before enabling one of them (or both), going through a full disable >>> makes little sense, even more if it triggers any sort of malfunctioning. >>> >>> Does this make sense to you? >> >> It also doesn't make too much sense to keep them both enabled when they >> don't need to be :-) You'll end up consuming more power. >> > > They've alwyas been up before introduction of dynamic routing, provided > something is connected to the TX source pad in DT. > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/adv748x/adv748x-core.c#L489 > > Power saving wise, we're not doing worse than before, and if that's a > concern, it should be identified first why the CSI-2 Tx PLL never gets > turned off: > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/adv748x/adv748x-core.c#L269 > See "mipi_pll_en, CSI-TXA Map, Address 0xDA[0]" registrer description. > > The two issues might be actually connected, I tried fixing this in the past, > but frame capture broke, and I didn't have time to investigate > fruther. > > To sum up, this patch solves an issue on some devices, it does not > perform worse than what we had from a power consumption perspective, > but I agree it might work around some deeper issues it might be worth > chasing. > > If I got your NAK on this, I'll keep carrying it in my tree when > testing with that device. > > Thanks > j > > >>>>> Fix this by preventing writing 0 to >>>>> ADV748X_IO_10 register, which gets only updated when links are enabled >>>>> again. >>>>> >>>>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") >>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> >>>>> --- >>>>> The issue presents itself only on some HDMI transmitters, and went unnoticed >>>>> during the development of: >>>>> "[PATCH v3 0/6] media: adv748x: Implement dynamic routing support" >>>>> >>>>> Patch intended to be applied on top of latest media-master, where the >>>>> "[PATCH v3 0/6] media: adv748x: Implement dynamic routing support" >>>>> series is applied. >>>>> >>>>> The patch reports a "Fixes" tag, but should actually be merged with the above >>>>> mentioned series. >>>>> >>>>> --- >>>>> drivers/media/i2c/adv748x/adv748x-core.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c >>>>> index f57cd77a32fa..0e5a75eb6d75 100644 >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>>>> @@ -354,6 +354,9 @@ static int adv748x_link_setup(struct media_entity *entity, >>>>> >>>>> tx->src = enable ? rsd : NULL; >>>>> >>>>> + if (!enable) >>>>> + return 0; >>>>> + >>>>> if (state->afe.tx) { >>>>> /* AFE Requires TXA enabled, even when output to TXB */ >>>>> io10 |= ADV748X_IO_10_CSI4_EN; >> >> -- >> Regards, >> >> Laurent Pinchart
