Re: [PATCH 2/3 v3] drm: bridge/dw-hdmi: Enable ISCR1, ISCR2 and ACP packets

[Jose Abreu <Jose.Abreu@synopsys.com>]

Hi Russell,


On 04-08-2016 16:32, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 11:44:50AM +0100, Jose Abreu wrote:
>> Currently ISCR and ACP packets are not being sent causing
>> HDMI compliance tests like CTS 7-19 HDMI 1.4b to fail.
> Hmm.  Reading the HDMI specifications (v1.3, being the publically
> available one), the specification does _not_ say that a source _must_
> transmit ISRC and ACP packets if Supports_AI is set.
>
> What it does say is that:
> * a sink shall set Supports_AI if it supports at least one function
>   which uses ISRC or ACP packets, and it shall accept such packets.
> * a source shall not transmit ISRC or ACP packets to a sink that
>   has Supports_AI=0.
>
> I don't see anything that mandates the transmission of ISRC and ACP
> packets when Supports_AI is set.
>
> Moreover, transmission of wrong ISRC information probably goes against
> the HDMI spec section 8.8, which requires specific ISRC status values
> in HB1 - I don't see how that _could_ be handled, as the kernel has no
> knowledge about the EAN of the audio content being played, nor when a
> track starts or finishes.
>
> Please clarify.

You are correct. I ran the test again and indeed it is not
failing but it is skipped. Sorry for the confusion. Anyway, with
this patch the test that verifies the ISRC packets succeeds so I
think it would be a nice addition.

>
>> +	/* Set ISCR1, ISCR2, and ACP packets to automatic scheduling */
>> +	if (hdmi->sink_supports_ai) {
>> +		dev_dbg(hdmi->dev, "sink supports AI packets\n");
>> +		hdmi_writeb(hdmi, 0x06, HDMI_FC_ISCR1_0);
>> +		hdmi_writeb(hdmi, 0x03, HDMI_FC_DATAUTO0);
>> +		hdmi_writeb(hdmi, 0x01, HDMI_FC_DATAUTO1);
>> +		hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> So what data gets sent in these packets - I guess whatever random data
> happens to be in the HDMI_FC_ACP* and HDMI_FC_ISCR1_* registers at the
> time the sending is enabled.  I don't think that's particularly a good
> idea.

It is not random as the registers defaults to zero per databook.
It is sending an empty packet.

>> @@ -1474,8 +1490,13 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>>  		/* Store the ELD */
>>  		drm_edid_to_eld(connector, edid);
>>  		kfree(edid);
>> +
>> +		hdmi->sink_supports_ai = connector->eld[5] & (0x1 << 1);
> We have a definition for this bit - DRM_ELD_SUPPORTS_AI.
>

Thanks. Will change in next version if you agree with this
addition. If not I will drop the patch.


Best regards,
Jose Miguel Abreu