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

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

[Jacopo Mondi]

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. 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@jmondi.org>
---
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.

Thanks
   j
---
 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;
--
2.20.1

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

[Laurent Pinchart]

Hi Jacopo,

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 ?

> 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@jmondi.org>
> ---
> 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

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3472 bytes --]

Hi Laurent,

On Wed, Mar 06, 2019 at 09:15:21PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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.

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?

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@jmondi.org>
> > ---
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ian Arkver]

Hi Jacopo,

On 07/03/2019 10:35, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Wed, Mar 06, 2019 at 09:15:21PM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> 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.

Maybe disabling the device is clearing the EDID RAM and the Chromecast 
rereads this, but the laptop doesn't? Just a thought.

Regards,
Ian.

> 
> 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?
> 
> 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@jmondi.org>
>>> ---
>>> 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

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

[Laurent Pinchart]

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 :-(

> 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.

> >> 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@jmondi.org>
> >> ---
> >> 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

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 5347 bytes --]

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.

> > 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@jmondi.org>
> > >> ---
> > >> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Hans Verkuil]

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@jmondi.org>
>>>>> ---
>>>>> 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

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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 6746 bytes --]

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@jmondi.org>
> >>>>> ---
> >>>>> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]