Re: [PATCH] media: adv748x: Don't disable CSI-2 on link_setup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux