Re: [PATCH 4/4] drm/vc4: add HDMI CEC support

[Eric Anholt <eric@anholt.net>]

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

Hans Verkuil <hverkuil@xs4all.nl> writes:

> On 12/07/17 21:43, Hans Verkuil wrote:
>> On 12/07/17 21:02, Eric Anholt wrote:
>>>> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>>> +				      u32 signal_free_time, struct cec_msg *msg)
>>>> +{
>>>> +	struct vc4_dev *vc4 = cec_get_drvdata(adap);
>>>> +	u32 val;
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < msg->len; i += 4)
>>>> +		HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
>>>> +			   (msg->msg[i]) |
>>>> +			   (msg->msg[i + 1] << 8) |
>>>> +			   (msg->msg[i + 2] << 16) |
>>>> +			   (msg->msg[i + 3] << 24));
>>>> +
>>>> +	val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>>>> +	val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
>>>> +	HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>>>> +	val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
>>>> +	val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
>>>> +	val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
>>>
>>> It doesn't look to me like len should have 1 subtracted from it.  The
>>> field has 4 bits for our up-to-16-byte length, and the firmware seems to
>>> be setting it to the same value as a memcpy for the message data uses.
>> 
>> You need to subtract by one. The CEC protocol supports messages of 1-16
>> bytes in length. Since the message length mask is only 4 bits you need to
>> encode this in the value 0-15. Hence the '-1', otherwise you would never
>> be able to send 16 byte messages.
>> 
>> I actually found this when debugging the messages it was transmitting: they
>> were one too long.
>> 
>> This suggests that the firmware does this wrong. I don't have time tomorrow,
>> but I'll see if I can do a quick test on Friday to verify that.
>
> I double-checked this and both the driver and the firmware do the right thing.
> Just to be certain I also tried sending a message that uses the full 16 byte
> payload and that too went well. So the code is definitely correct.

Great, I'll assume that I just missed the subtraction somewhere in the
layers of firmware code.  Thanks for checking!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]