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

[Ian Arkver <ian.arkver.dev@gmail.com>]

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