Hans Verkuil <hverkuil@xxxxxxxxx> 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:
signature.asc
Description: PGP signature
