Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410443 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when  EDID checksum fails. Joao Pinto <Joao.Pinto@synopsys.com> - 2016-05-31 19:10 +0200
    Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when  EDID checksum fails. Joao Pinto <Joao.Pinto@synopsys.com> - 2016-06-01 18:40 +0200
    Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when  EDID checksum fails. Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-06-01 18:40 +0200

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

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.

Could you please check the sound support patch?

Thanks,
Joao

[toc] | [next] | [standalone]

#1411389

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

[toc] | [prev] | [next] | [standalone]

#1411394

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.

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?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web