diff mbox

[media] cec: GIVE_PHYSICAL_ADDR should respond to unregistered device

Message ID 73019b13e5e8d727c37ec1b99f2e746aad0a7153.1505388690.git.joabreu@synopsys.com (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Jose Abreu Sept. 14, 2017, 11:33 a.m. UTC 
  Running CEC 1.4 compliance test we get the following error on test
11.1.6.2: "ERROR: The DUT did not broadcast a
<Report Physical Address> message to the unregistered device."

Fix this by letting GIVE_PHYSICAL_ADDR message respond to unregistered
device.

With this fix we pass CEC 1.4 official compliance.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Joao Pinto <jpinto@synopsys.com>
---
 drivers/media/cec/cec-adap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Hans Verkuil (hansverk) Sept. 14, 2017, 1:10 p.m. UTC | #1 
On 09/14/17 13:33, Jose Abreu wrote:
> Running CEC 1.4 compliance test we get the following error on test
> 11.1.6.2: "ERROR: The DUT did not broadcast a
> <Report Physical Address> message to the unregistered device."
> 
> Fix this by letting GIVE_PHYSICAL_ADDR message respond to unregistered
> device.
> 
> With this fix we pass CEC 1.4 official compliance.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> ---
>  drivers/media/cec/cec-adap.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index dd769e4..48482aa 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -1797,9 +1797,12 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>  	case CEC_MSG_GIVE_DEVICE_VENDOR_ID:
>  	case CEC_MSG_ABORT:
>  	case CEC_MSG_GIVE_DEVICE_POWER_STATUS:
> -	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>  	case CEC_MSG_GIVE_OSD_NAME:
>  	case CEC_MSG_GIVE_FEATURES:
> +		if (from_unregistered)

This should be (!adap->passthrough && from_unregistered)

> +			return 0;

Actually, CEC_MSG_GIVE_DEVICE_VENDOR_ID and CEC_MSG_GIVE_FEATURES
fall in the same category as CEC_MSG_GIVE_PHYSICAL_ADDR. I.e. these are
directed messages but the reply is a broadcast message. All three can be
sent by an unregistered device. It's a good idea to mention this here.
I.e. something like:

		/* These messages reply with a directed message, so ignore if
		   the initiator is Unregistered */

> +		/* Fall through */
> +	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>  		/*
>  		 * Skip processing these messages if the passthrough mode
>  		 * is on.
> @@ -1807,7 +1810,7 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>  		if (adap->passthrough)
>  			goto skip_processing;
>  		/* Ignore if addressing is wrong */
> -		if (is_broadcast || from_unregistered)
> +		if (is_broadcast)
>  			return 0;
>  		break;
>  
> 

Good catch, if you can make a v2 then I'll get this in for 4.14.

Not bad, just one obscure compliance error!

Regards,

	Hans
Jose Abreu Sept. 14, 2017, 1:28 p.m. UTC | #2 
Hi Hans,

On 14-09-2017 14:10, Hans Verkuil wrote:
> On 09/14/17 13:33, Jose Abreu wrote:
>> Running CEC 1.4 compliance test we get the following error on test
>> 11.1.6.2: "ERROR: The DUT did not broadcast a
>> <Report Physical Address> message to the unregistered device."
>>
>> Fix this by letting GIVE_PHYSICAL_ADDR message respond to unregistered
>> device.
>>
>> With this fix we pass CEC 1.4 official compliance.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> ---
>>  drivers/media/cec/cec-adap.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
>> index dd769e4..48482aa 100644
>> --- a/drivers/media/cec/cec-adap.c
>> +++ b/drivers/media/cec/cec-adap.c
>> @@ -1797,9 +1797,12 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>>  	case CEC_MSG_GIVE_DEVICE_VENDOR_ID:
>>  	case CEC_MSG_ABORT:
>>  	case CEC_MSG_GIVE_DEVICE_POWER_STATUS:
>> -	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>>  	case CEC_MSG_GIVE_OSD_NAME:
>>  	case CEC_MSG_GIVE_FEATURES:
>> +		if (from_unregistered)
> This should be (!adap->passthrough && from_unregistered)

Ok.

>
>> +			return 0;
> Actually, CEC_MSG_GIVE_DEVICE_VENDOR_ID and CEC_MSG_GIVE_FEATURES
> fall in the same category as CEC_MSG_GIVE_PHYSICAL_ADDR. I.e. these are
> directed messages but the reply is a broadcast message. All three can be
> sent by an unregistered device. It's a good idea to mention this here.
> I.e. something like:
>
> 		/* These messages reply with a directed message, so ignore if
> 		   the initiator is Unregistered */

Ok, but this comment applies to the remaining msgs, right? I
mean, GIVE_DEVICE_VENDOR_ID, GIVE_FEATURES and GIVE_PHYSICAL_ADDR
will still send a response if initiator is unregistered, correct?

>
>> +		/* Fall through */
>> +	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>>  		/*
>>  		 * Skip processing these messages if the passthrough mode
>>  		 * is on.
>> @@ -1807,7 +1810,7 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>>  		if (adap->passthrough)
>>  			goto skip_processing;
>>  		/* Ignore if addressing is wrong */
>> -		if (is_broadcast || from_unregistered)
>> +		if (is_broadcast)
>>  			return 0;
>>  		break;
>>  
>>
> Good catch, if you can make a v2 then I'll get this in for 4.14.
>
> Not bad, just one obscure compliance error!

Actually, I have at least one more fix which I don't know if it's
valid and I didn't manage to actually write it in a nice way.
This one is for CEC 2.0. My test equipment (which is certified)
in some tests sends msgs only with the opcode. As the received
msg length does not match the expected one CEC core is rejecting
the message and my compliance test is failling (test is 4.2.3).
Have you run this test? Did it pass?

Best regards,
Jose Miguel Abreu

>
> Regards,
>
> 	Hans
Hans Verkuil Sept. 14, 2017, 3:09 p.m. UTC | #3 
On 09/14/17 15:28, Jose Abreu wrote:
> Hi Hans,
> 
> On 14-09-2017 14:10, Hans Verkuil wrote:
>> On 09/14/17 13:33, Jose Abreu wrote:
>>> Running CEC 1.4 compliance test we get the following error on test
>>> 11.1.6.2: "ERROR: The DUT did not broadcast a
>>> <Report Physical Address> message to the unregistered device."
>>>
>>> Fix this by letting GIVE_PHYSICAL_ADDR message respond to unregistered
>>> device.
>>>
>>> With this fix we pass CEC 1.4 official compliance.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>>> Cc: Joao Pinto <jpinto@synopsys.com>
>>> ---
>>>  drivers/media/cec/cec-adap.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
>>> index dd769e4..48482aa 100644
>>> --- a/drivers/media/cec/cec-adap.c
>>> +++ b/drivers/media/cec/cec-adap.c
>>> @@ -1797,9 +1797,12 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>>>  	case CEC_MSG_GIVE_DEVICE_VENDOR_ID:
>>>  	case CEC_MSG_ABORT:
>>>  	case CEC_MSG_GIVE_DEVICE_POWER_STATUS:
>>> -	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>>>  	case CEC_MSG_GIVE_OSD_NAME:
>>>  	case CEC_MSG_GIVE_FEATURES:
>>> +		if (from_unregistered)
>> This should be (!adap->passthrough && from_unregistered)
> 
> Ok.
> 
>>
>>> +			return 0;
>> Actually, CEC_MSG_GIVE_DEVICE_VENDOR_ID and CEC_MSG_GIVE_FEATURES
>> fall in the same category as CEC_MSG_GIVE_PHYSICAL_ADDR. I.e. these are
>> directed messages but the reply is a broadcast message. All three can be
>> sent by an unregistered device. It's a good idea to mention this here.
>> I.e. something like:
>>
>> 		/* These messages reply with a directed message, so ignore if
>> 		   the initiator is Unregistered */
> 
> Ok, but this comment applies to the remaining msgs, right? I
> mean, GIVE_DEVICE_VENDOR_ID, GIVE_FEATURES and GIVE_PHYSICAL_ADDR
> will still send a response if initiator is unregistered, correct?

No, I meant this to go right before the 'if (!adap->passthrough && from_unregistered)'
statement. Sorry for the confusion.

> 
>>
>>> +		/* Fall through */
>>> +	case CEC_MSG_GIVE_PHYSICAL_ADDR:
>>>  		/*
>>>  		 * Skip processing these messages if the passthrough mode
>>>  		 * is on.
>>> @@ -1807,7 +1810,7 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
>>>  		if (adap->passthrough)
>>>  			goto skip_processing;
>>>  		/* Ignore if addressing is wrong */
>>> -		if (is_broadcast || from_unregistered)
>>> +		if (is_broadcast)
>>>  			return 0;
>>>  		break;
>>>  
>>>
>> Good catch, if you can make a v2 then I'll get this in for 4.14.
>>
>> Not bad, just one obscure compliance error!
> 
> Actually, I have at least one more fix which I don't know if it's
> valid and I didn't manage to actually write it in a nice way.
> This one is for CEC 2.0. My test equipment (which is certified)
> in some tests sends msgs only with the opcode. As the received
> msg length does not match the expected one CEC core is rejecting
> the message and my compliance test is failling (test is 4.2.3).

In the HDMI 1.4 spec in CEC 7.3 (Frame Validation) it says that a follower
should ignore frames that are too small.

At the same time unsupported opcodes should result in a Feature Abort.

If you don't send a properly formed message, then it's not clear to me
what should happen. Which opcode failed?

> Have you run this test? Did it pass?

No, we haven't. Never got around to that.

Regards,

	Hans

> 
> Best regards,
> Jose Miguel Abreu
> 
>>
>> Regards,
>>
>> 	Hans
>
Jose Abreu Sept. 14, 2017, 3:30 p.m. UTC | #4 
On 14-09-2017 16:09, Hans Verkuil wrote:
> On 09/14/17 15:28, Jose Abreu wrote:
>
>> Actually, I have at least one more fix which I don't know if it's
>> valid and I didn't manage to actually write it in a nice way.
>> This one is for CEC 2.0. My test equipment (which is certified)
>> in some tests sends msgs only with the opcode. As the received
>> msg length does not match the expected one CEC core is rejecting
>> the message and my compliance test is failling (test is 4.2.3).
> In the HDMI 1.4 spec in CEC 7.3 (Frame Validation) it says that a follower
> should ignore frames that are too small.
>
> At the same time unsupported opcodes should result in a Feature Abort.
>
> If you don't send a properly formed message, then it's not clear to me
> what should happen. Which opcode failed?

Hmm, yeah, the spec confirms. The failing opcodes are the ones
that have arguments, the test equipment is just sending the
header plus opcode. Anyway, for this failing test the MOI for
this equipment is not approved so I will probably carry this fix
only locally and send it upstream only if the MOI gets approved.

>
>> Have you run this test? Did it pass?
> No, we haven't. Never got around to that.

Ok. I can say that CEC 1.4 + CEC 2.0 all pass compliance with
this patch and with my local fix + my test app!

Best regards,
Jose Miguel Abreu

>
> Regards,
>
> 	Hans
>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> Regards,
>>>
>>> 	Hans
Hans Verkuil Sept. 14, 2017, 3:46 p.m. UTC | #5 
On 09/14/2017 05:30 PM, Jose Abreu wrote:
> 
> 
> On 14-09-2017 16:09, Hans Verkuil wrote:
>> On 09/14/17 15:28, Jose Abreu wrote:
>>
>>> Actually, I have at least one more fix which I don't know if it's
>>> valid and I didn't manage to actually write it in a nice way.
>>> This one is for CEC 2.0. My test equipment (which is certified)
>>> in some tests sends msgs only with the opcode. As the received
>>> msg length does not match the expected one CEC core is rejecting
>>> the message and my compliance test is failling (test is 4.2.3).
>> In the HDMI 1.4 spec in CEC 7.3 (Frame Validation) it says that a follower
>> should ignore frames that are too small.
>>
>> At the same time unsupported opcodes should result in a Feature Abort.
>>
>> If you don't send a properly formed message, then it's not clear to me
>> what should happen. Which opcode failed?
> 
> Hmm, yeah, the spec confirms. The failing opcodes are the ones
> that have arguments, the test equipment is just sending the
> header plus opcode. Anyway, for this failing test the MOI for
> this equipment is not approved so I will probably carry this fix
> only locally and send it upstream only if the MOI gets approved.

So is this test just running through all opcodes from 1-255 and see
what happens? Or is it only doing this for a subset of opcodes?
And if so, which?

I'm just curious to see how this is done.

Which test equipment do you use?

We don't actually have any certified CEC 2.0 test equipment, only 1.4,
so I'm grateful that you did it for me :-)

Regards,

	Hans

> 
>>
>>> Have you run this test? Did it pass?
>> No, we haven't. Never got around to that.
> 
> Ok. I can say that CEC 1.4 + CEC 2.0 all pass compliance with
> this patch and with my local fix + my test app!
> 
> Best regards,
> Jose Miguel Abreu
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> Best regards,
>>> Jose Miguel Abreu
>>>
>>>> Regards,
>>>>
>>>> 	Hans
>
Jose Abreu Sept. 15, 2017, 10:34 a.m. UTC | #6 
Hi Hans,

On 14-09-2017 16:46, Hans Verkuil wrote:
> On 09/14/2017 05:30 PM, Jose Abreu wrote:
>>
>> On 14-09-2017 16:09, Hans Verkuil wrote:
>>> On 09/14/17 15:28, Jose Abreu wrote:
>>>
>>>> Actually, I have at least one more fix which I don't know if it's
>>>> valid and I didn't manage to actually write it in a nice way.
>>>> This one is for CEC 2.0. My test equipment (which is certified)
>>>> in some tests sends msgs only with the opcode. As the received
>>>> msg length does not match the expected one CEC core is rejecting
>>>> the message and my compliance test is failling (test is 4.2.3).
>>> In the HDMI 1.4 spec in CEC 7.3 (Frame Validation) it says that a follower
>>> should ignore frames that are too small.
>>>
>>> At the same time unsupported opcodes should result in a Feature Abort.
>>>
>>> If you don't send a properly formed message, then it's not clear to me
>>> what should happen. Which opcode failed?
>> Hmm, yeah, the spec confirms. The failing opcodes are the ones
>> that have arguments, the test equipment is just sending the
>> header plus opcode. Anyway, for this failing test the MOI for
>> this equipment is not approved so I will probably carry this fix
>> only locally and send it upstream only if the MOI gets approved.
> So is this test just running through all opcodes from 1-255 and see
> what happens? Or is it only doing this for a subset of opcodes?
> And if so, which?

Its running through the opcodes that the sink does not support
and sending only the opcode. By spec sink should respond with
Feature Abort. But, as you said, only sending opcode is out of
spec (for opcodes that have arguments).

>
> I'm just curious to see how this is done.
>
> Which test equipment do you use?

Its a QD980.

Best regards,
Jose Miguel Abreu

>
> We don't actually have any certified CEC 2.0 test equipment, only 1.4,
> so I'm grateful that you did it for me :-)
>
> Regards,
>
> 	Hans
>
>>>> Have you run this test? Did it pass?
>>> No, we haven't. Never got around to that.
>> Ok. I can say that CEC 1.4 + CEC 2.0 all pass compliance with
>> this patch and with my local fix + my test app!
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>> Best regards,
>>>> Jose Miguel Abreu
>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
diff mbox

Patch

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index dd769e4..48482aa 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -1797,9 +1797,12 @@  static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
 	case CEC_MSG_GIVE_DEVICE_VENDOR_ID:
 	case CEC_MSG_ABORT:
 	case CEC_MSG_GIVE_DEVICE_POWER_STATUS:
-	case CEC_MSG_GIVE_PHYSICAL_ADDR:
 	case CEC_MSG_GIVE_OSD_NAME:
 	case CEC_MSG_GIVE_FEATURES:
+		if (from_unregistered)
+			return 0;
+		/* Fall through */
+	case CEC_MSG_GIVE_PHYSICAL_ADDR:
 		/*
 		 * Skip processing these messages if the passthrough mode
 		 * is on.
@@ -1807,7 +1810,7 @@  static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
 		if (adap->passthrough)
 			goto skip_processing;
 		/* Ignore if addressing is wrong */
-		if (is_broadcast || from_unregistered)
+		if (is_broadcast)
 			return 0;
 		break;