[PATCH 0/2] video/hdmi: two fixes

[PATCH 0/2] video/hdmi: two fixes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Ville, can you add these two to your "drm/edid: Infoframe cleanups and fixes"
patch series? These two have been in our queue for some time now and were
never upstreamed, so this is a good opportunity to finally kick them out.

Once all this is merged I really should sit down and add full HDMI 2.0
and CTA-861-G support as well.

Regards,

	Hans

Hans Verkuil (1):
  drivers/video/hdmi: allow for larger-than-needed vendor IF

Martin Bugge (1):
  hdmi: audio infoframe log: corrected channel count

 drivers/video/hdmi.c | 5 ++---
 include/linux/hdmi.h | 1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.14.1

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

[Hans Verkuil]

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(-)

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

[PATCH 2/2] hdmi: audio infoframe log: corrected channel count

[Hans Verkuil]

From: Martin Bugge <marbugge@cisco.com>

Audio channel count should start from 2.

Reference: CEA-861-F Table 27.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Reported-by: Ahung Cheng <ahcheng@nvidia.com>
Signed-off-by: Martin Bugge <marbugge@cisco.com>
---
 drivers/video/hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 61f803f75a47..5f78b254dd59 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -884,7 +884,7 @@ static void hdmi_audio_infoframe_log(const char *level,
 				  (struct hdmi_any_infoframe *)frame);
 
 	if (frame->channels)
-		hdmi_log("    channels: %u\n", frame->channels - 1);
+		hdmi_log("    channels: %u\n", frame->channels + 1);
 	else
 		hdmi_log("    channels: Refer to stream header\n");
 	hdmi_log("    coding type: %s\n",
-- 
2.14.1

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

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

[Hans Verkuil]

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
> 

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

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

[Hans Verkuil]

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