Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

From: Joao Pinto <Joao.Pinto@synopsys.com>
To: Russell King - ARM Linux <linux@armlinux.org.uk>,
	Joao Pinto <Joao.Pinto@synopsys.com>
Cc: <linux-kernel@vger.kernel.org>, <ijc+devicetree@hellion.org.uk>,
	<liviu.dudau@arm.com>, <ryan.harkin@linaro.org>
Subject: Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.
Date: Wed, 1 Jun 2016 17:38:41 +0100	[thread overview]
Message-ID: <ea7d998c-55d0-fb42-2f24-6023a283d734@synopsys.com> (raw)
In-Reply-To: <20160601163228.GF19428@n2100.arm.linux.org.uk>

On 6/1/2016 5:32 PM, Russell King - ARM Linux wrote:
> On Tue, May 31, 2016 at 05:58:31PM +0100, Joao Pinto wrote:
>> Hi Russell,
>>
>> On 5/30/2016 8:10 PM, Russell King - ARM Linux wrote:
>>> On Mon, May 30, 2016 at 04:15:54PM +0100, Joao Pinto wrote:
>>>> When using ffplay to reproduce video+sound it was noticed that sometimes the
>>>> sound was disabled. The cause was an initial EDID checksum error that disabled
>>
>> (...)
>>
>>>> @@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
>>>>  
>>>>  		/* init read EDID waitqueue and HDP work */
>>>>  		init_waitqueue_head(&priv->wq_edid);
>>>> +		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
>>>>  
>>>>  		/* clear pending interrupts */
>>>>  		reg_read(priv, REG_INT_FLAGS_0);
>>>
>>> Clearly, this patch is incomplete.  There's nothing that schedules this
>>> work to be run.
>>
>> You are right, forgot to include the schedule in the patch!
>>
>>>
>>> In any case, this is reintroducing the code which I deleted when I fixed
>>> the (rather crappy) previous implemention of delaying the EDID read after
>>> a hotplug event.  You should not need this patch.
>>>
>>
>> If a checksum validation fails the video reproduction is done muted if
>> you use a simple app like ffplay. This does not happen if using mplayer.
> 
> So?
> 
> We already delay the EDID read to work around reading a corrupted EDID.
> If you need an additional delay, then it seems that the existing delay
> is not long enough, and maybe we should extend it a little more.

I increased the delay to 200ms (x2) and the problem persisted.

> 
> What we should not do is to delay the HPD signalling.  That is definitely
> the wrong solution.
> 
> In any case, I'm having a hard time understanding what the problem here
> is.  You've unplugged the connected HDMI sink, so there's no longer a
> display device connected, and the display hardware is shut down.  The
> EDID becomes invalid.  You then plug in a HDMI sink, and now you have a
> display device connected.  The EDID is read, and the display hardware is
> configured according to the EDID details.  Video output is resumed, and
> it takes a second or so for the sink to lock on and display the resulting
> video.
> 
> At what point does the problem occur?

The problem happens at the kernel boot. If when the driver is instantiated the
EDID checksum is ok, ffplay plays well video+sound. If the checksum is not ok,
it assumes that the output is DVI (I think that this is the reason why we don't
have video output with sound), producing video without sound.
I have tested this and it is always the same.

If you use mplayer, the problem does not happen, since the app surely makes some
tweak that leads the driver to enable the sound.

Joao