Hi Hans, On Mon, Mar 11, 2019 at 03:05:24PM +0100, Hans Verkuil wrote: > 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. Thanks for adding more details to the issue. Even if not related to the CSI ports directly, the issue triggers when all of the two TXes gets disabled, and this patch prevents that from happening. I cannot elaborate a detailed explanation of the reasons why, so I understand if you prefer to ditch this patch. In my opinion is better to have something that fix the issue and makes the adv748x work with a broader number of transmitters than not, but I let you and Laurent decide if to take this one in. Thanks j > > 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 >
Attachment:
signature.asc
Description: PGP signature
