[1/2] drivers/video/hdmi: allow for larger-than-needed vendor IF

Submitted by Hans Verkuil on Nov. 20, 2017, 1:41 p.m.

Details

Message ID 20171120134129.26161-2-hverkuil@xs4all.nl
State New
Headers show
Series "video/hdmi: two fixes" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Hans Verkuil Nov. 20, 2017, 1:41 p.m.
From: Hans Verkuil <hansverk@cisco.com>

Some devices (Windows Intel driver!) send a Vendor InfoFrame that
uses a payload length of 0x1b instead of the length of 5 or 6
that the unpack code expects. The InfoFrame is padded with 0 by
the source.

The current code thinks anything other than 5 or 6 is an error,
but larger values are allowed by the specification. So support
that here as well.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/video/hdmi.c | 3 +--
 include/linux/hdmi.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 1cf907ecded4..61f803f75a47 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -1164,8 +1164,7 @@  hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
 	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
 
 	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
-	    ptr[1] != 1 ||
-	    (ptr[2] != 5 && ptr[2] != 6))
+	    ptr[1] != 1 || ptr[2] < 5 || ptr[2] > HDMI_VENDOR_INFOFRAME_SIZE)
 		return -EINVAL;
 
 	length = ptr[2];
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index d271ff23984f..14d3531a0eda 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -40,6 +40,7 @@  enum hdmi_infoframe_type {
 #define HDMI_AVI_INFOFRAME_SIZE    13
 #define HDMI_SPD_INFOFRAME_SIZE    25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
+#define HDMI_VENDOR_INFOFRAME_SIZE 31
 
 #define HDMI_INFOFRAME_SIZE(type)	\
 	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)

Comments

On Mon, Nov 20, 2017 at 02:41:28PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> Some devices (Windows Intel driver!) send a Vendor InfoFrame that
> uses a payload length of 0x1b instead of the length of 5 or 6
> that the unpack code expects. The InfoFrame is padded with 0 by
> the source.

So it doesn't put any 3D_Metadata stuff in there? We don't see to
have code to parse/generate any of that.

Sadly the spec doesn't seem to forbid sending an overly long infoframe
as long it's padded with 0. Would have been nicer for extending it if
that sort of thing was forbidden. But I guess everything can be solved
with flags. Not that I expect anyone to extend it anymore now that
HDMI 2.0 has specified a totally new infoframe.

> 
> The current code thinks anything other than 5 or 6 is an error,
> but larger values are allowed by the specification. So support
> that here as well.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/video/hdmi.c | 3 +--
>  include/linux/hdmi.h | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 1cf907ecded4..61f803f75a47 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1164,8 +1164,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>  	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
>  
>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
> -	    ptr[1] != 1 ||
> -	    (ptr[2] != 5 && ptr[2] != 6))
> +	    ptr[1] != 1 || ptr[2] < 5 || ptr[2] > HDMI_VENDOR_INFOFRAME_SIZE)
>  		return -EINVAL;
>  
>  	length = ptr[2];
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index d271ff23984f..14d3531a0eda 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -40,6 +40,7 @@ enum hdmi_infoframe_type {
>  #define HDMI_AVI_INFOFRAME_SIZE    13
>  #define HDMI_SPD_INFOFRAME_SIZE    25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
> +#define HDMI_VENDOR_INFOFRAME_SIZE 31
>  
>  #define HDMI_INFOFRAME_SIZE(type)	\
>  	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
> -- 
> 2.14.1
On 11/20/2017 03:51 PM, Ville Syrjälä wrote:
> On Mon, Nov 20, 2017 at 02:41:28PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> Some devices (Windows Intel driver!) send a Vendor InfoFrame that
>> uses a payload length of 0x1b instead of the length of 5 or 6
>> that the unpack code expects. The InfoFrame is padded with 0 by
>> the source.
> 
> So it doesn't put any 3D_Metadata stuff in there? We don't see to
> have code to parse/generate any of that.

I can't remember if it puts any 3D stuff in there. Let me know if you
want me to check this later this week.

> Sadly the spec doesn't seem to forbid sending an overly long infoframe
> as long it's padded with 0. Would have been nicer for extending it if
> that sort of thing was forbidden. But I guess everything can be solved
> with flags. Not that I expect anyone to extend it anymore now that
> HDMI 2.0 has specified a totally new infoframe.
> 
>>
>> The current code thinks anything other than 5 or 6 is an error,
>> but larger values are allowed by the specification. So support
>> that here as well.

Can you add this line here (I thought I mentioned it, but it appears
I forgot it):

"Support for larger Vendor InfoFrames is also needed for HDMI_FORUM_IEEE_OUI
Vendor InfoFrames."

Thank you,

	Hans

>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> ---
>>  drivers/video/hdmi.c | 3 +--
>>  include/linux/hdmi.h | 1 +
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>> index 1cf907ecded4..61f803f75a47 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -1164,8 +1164,7 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
>>  	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
>>  
>>  	if (ptr[0] != HDMI_INFOFRAME_TYPE_VENDOR ||
>> -	    ptr[1] != 1 ||
>> -	    (ptr[2] != 5 && ptr[2] != 6))
>> +	    ptr[1] != 1 || ptr[2] < 5 || ptr[2] > HDMI_VENDOR_INFOFRAME_SIZE)
>>  		return -EINVAL;
>>  
>>  	length = ptr[2];
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
>> index d271ff23984f..14d3531a0eda 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -40,6 +40,7 @@ enum hdmi_infoframe_type {
>>  #define HDMI_AVI_INFOFRAME_SIZE    13
>>  #define HDMI_SPD_INFOFRAME_SIZE    25
>>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>> +#define HDMI_VENDOR_INFOFRAME_SIZE 31
>>  
>>  #define HDMI_INFOFRAME_SIZE(type)	\
>>  	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
>> -- 
>> 2.14.1
>
On Mon, Nov 20, 2017 at 04:02:14PM +0100, Hans Verkuil wrote:
> On 11/20/2017 03:51 PM, Ville Syrjälä wrote:
> > On Mon, Nov 20, 2017 at 02:41:28PM +0100, Hans Verkuil wrote:
> >> From: Hans Verkuil <hansverk@cisco.com>
> >>
> >> Some devices (Windows Intel driver!) send a Vendor InfoFrame that
> >> uses a payload length of 0x1b instead of the length of 5 or 6
> >> that the unpack code expects. The InfoFrame is padded with 0 by
> >> the source.
> > 
> > So it doesn't put any 3D_Metadata stuff in there? We don't see to
> > have code to parse/generate any of that.
> 
> I can't remember if it puts any 3D stuff in there. Let me know if you
> want me to check this later this week.

Would be nice to know.

I guess we should really add code to parse/generate that stuff too,
otherwise we're going to be lying when we unpack an infoframe with that
stuff present.
On 11/21/17 16:27, Ville Syrjälä wrote:
> On Mon, Nov 20, 2017 at 04:02:14PM +0100, Hans Verkuil wrote:
>> On 11/20/2017 03:51 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 20, 2017 at 02:41:28PM +0100, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>
>>>> Some devices (Windows Intel driver!) send a Vendor InfoFrame that
>>>> uses a payload length of 0x1b instead of the length of 5 or 6
>>>> that the unpack code expects. The InfoFrame is padded with 0 by
>>>> the source.
>>>
>>> So it doesn't put any 3D_Metadata stuff in there? We don't see to
>>> have code to parse/generate any of that.
>>
>> I can't remember if it puts any 3D stuff in there. Let me know if you
>> want me to check this later this week.
> 
> Would be nice to know.
> 
> I guess we should really add code to parse/generate that stuff too,
> otherwise we're going to be lying when we unpack an infoframe with that
> stuff present.
> 

Hmm, I can't reproduce this anymore. We did observe it but I can't
remember with which hardware or graphics driver version.

Testing with a Windows 10 Intel laptop with recent drivers just showed
either an empty vendor InfoFrame with a 1080p EDID or a vendor InfoFrame
that sets the HDMI VIC to 1 for a 4kp30 EDID.

The length is 5 in both cases.

Testing with a different laptop gave a vendor InfoFrame with HDMI VIC 1 and
a length of 6.

Regards,

	Hans