[PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[Derek Foreman]

The largest infoframe we create is the DRM (Dynamic Range Mastering)
infoframe which is 26 bytes + a 4 byte header, for a total of 30
bytes.

With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we
allocate too little space to pack a DRM infoframe in
write_device_infoframe(), leading to an ENOSPC return from
hdmi_infoframe_pack(), and never calling the connector's
write_infoframe() vfunc.

Instead of having HDMI_MAX_INFOFRAME_SIZE defined in two places,
replace HDMI_MAX_INFOFRAME_SIZE with HDMI_INFOFRAME_SIZE(MAX) and make
MAX the same size as the DRM infoframe.

Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 +---
 drivers/gpu/drm/drm_debugfs.c                   | 4 +---
 include/linux/hdmi.h                            | 3 +++
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 7854820089ec..feb7a3a75981 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -521,8 +521,6 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
 
-#define HDMI_MAX_INFOFRAME_SIZE		29
-
 static int clear_device_infoframe(struct drm_connector *connector,
 				  enum hdmi_infoframe_type type)
 {
@@ -563,7 +561,7 @@ static int write_device_infoframe(struct drm_connector *connector,
 {
 	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
 	struct drm_device *dev = connector->dev;
-	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
+	u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
 	int ret;
 	int len;
 
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b239a24f1df..9d3e6dd68810 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -520,8 +520,6 @@ static const struct file_operations drm_connector_fops = {
 	.write = connector_write
 };
 
-#define HDMI_MAX_INFOFRAME_SIZE		29
-
 static ssize_t
 audio_infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
 {
@@ -579,7 +577,7 @@ static ssize_t _f##_read_infoframe(struct file *filp, \
 	struct drm_connector *connector; \
 	union hdmi_infoframe *frame; \
 	struct drm_device *dev; \
-	u8 buf[HDMI_MAX_INFOFRAME_SIZE]; \
+	u8 buf[HDMI_INFOFRAME_SIZE(MAX)]; \
 	ssize_t len = 0; \
 	\
 	connector = filp->private_data; \
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 3bb87bf6bc65..3a442a59919e 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -59,6 +59,9 @@ enum hdmi_infoframe_type {
 #define HDMI_DRM_INFOFRAME_SIZE    26
 #define HDMI_VENDOR_INFOFRAME_SIZE  4
 
+/* The biggest infoframe size */
+#define HDMI_MAX_INFOFRAME_SIZE		HDMI_DRM_INFOFRAME_SIZE
+
 #define HDMI_INFOFRAME_SIZE(type)	\
 	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
 
-- 
2.45.2

Re: [PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[Jani Nikula]

On Mon, 26 Aug 2024, Derek Foreman <derek.foreman@collabora.com> wrote:
> The largest infoframe we create is the DRM (Dynamic Range Mastering)
> infoframe which is 26 bytes + a 4 byte header, for a total of 30
> bytes.
>
> With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we
> allocate too little space to pack a DRM infoframe in
> write_device_infoframe(), leading to an ENOSPC return from
> hdmi_infoframe_pack(), and never calling the connector's
> write_infoframe() vfunc.
>
> Instead of having HDMI_MAX_INFOFRAME_SIZE defined in two places,
> replace HDMI_MAX_INFOFRAME_SIZE with HDMI_INFOFRAME_SIZE(MAX) and make
> MAX the same size as the DRM infoframe.

Fixes: f378b77227bc ("drm/connector: hdmi: Add Infoframes generation")
Fixes: c602e4959a0c ("drm/connector: hdmi: Create Infoframe DebugFS entries")
Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 +---
>  drivers/gpu/drm/drm_debugfs.c                   | 4 +---
>  include/linux/hdmi.h                            | 3 +++

This file doesn't actually have a MAINTAINERS entry, Cc: Helge and
linux-fbdev for good measure because it really corresponds to
drivers/video/hdmi.c. But it's benign, should be fine to merge via
drm-misc.

BR,
Jani.


>  3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 7854820089ec..feb7a3a75981 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -521,8 +521,6 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
>  
> -#define HDMI_MAX_INFOFRAME_SIZE		29
> -
>  static int clear_device_infoframe(struct drm_connector *connector,
>  				  enum hdmi_infoframe_type type)
>  {
> @@ -563,7 +561,7 @@ static int write_device_infoframe(struct drm_connector *connector,
>  {
>  	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
>  	struct drm_device *dev = connector->dev;
> -	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
> +	u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
>  	int ret;
>  	int len;
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..9d3e6dd68810 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -520,8 +520,6 @@ static const struct file_operations drm_connector_fops = {
>  	.write = connector_write
>  };
>  
> -#define HDMI_MAX_INFOFRAME_SIZE		29
> -
>  static ssize_t
>  audio_infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
>  {
> @@ -579,7 +577,7 @@ static ssize_t _f##_read_infoframe(struct file *filp, \
>  	struct drm_connector *connector; \
>  	union hdmi_infoframe *frame; \
>  	struct drm_device *dev; \
> -	u8 buf[HDMI_MAX_INFOFRAME_SIZE]; \
> +	u8 buf[HDMI_INFOFRAME_SIZE(MAX)]; \
>  	ssize_t len = 0; \
>  	\
>  	connector = filp->private_data; \
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 3bb87bf6bc65..3a442a59919e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -59,6 +59,9 @@ enum hdmi_infoframe_type {
>  #define HDMI_DRM_INFOFRAME_SIZE    26
>  #define HDMI_VENDOR_INFOFRAME_SIZE  4
>  
> +/* The biggest infoframe size */
> +#define HDMI_MAX_INFOFRAME_SIZE		HDMI_DRM_INFOFRAME_SIZE
> +
>  #define HDMI_INFOFRAME_SIZE(type)	\
>  	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)

-- 
Jani Nikula, Intel

Re: [PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[Maxime Ripard]

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

Hi,

On Mon, Aug 26, 2024 at 07:10:11AM GMT, Derek Foreman wrote:
> The largest infoframe we create is the DRM (Dynamic Range Mastering)
> infoframe which is 26 bytes + a 4 byte header, for a total of 30
> bytes.
> 
> With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we
> allocate too little space to pack a DRM infoframe in
> write_device_infoframe(), leading to an ENOSPC return from
> hdmi_infoframe_pack(), and never calling the connector's
> write_infoframe() vfunc.
> 
> Instead of having HDMI_MAX_INFOFRAME_SIZE defined in two places,
> replace HDMI_MAX_INFOFRAME_SIZE with HDMI_INFOFRAME_SIZE(MAX) and make
> MAX the same size as the DRM infoframe.
> 
> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 +---
>  drivers/gpu/drm/drm_debugfs.c                   | 4 +---
>  include/linux/hdmi.h                            | 3 +++
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 7854820089ec..feb7a3a75981 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -521,8 +521,6 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
>  
> -#define HDMI_MAX_INFOFRAME_SIZE		29
> -
>  static int clear_device_infoframe(struct drm_connector *connector,
>  				  enum hdmi_infoframe_type type)
>  {
> @@ -563,7 +561,7 @@ static int write_device_infoframe(struct drm_connector *connector,
>  {
>  	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
>  	struct drm_device *dev = connector->dev;
> -	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
> +	u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
>  	int ret;
>  	int len;
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..9d3e6dd68810 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -520,8 +520,6 @@ static const struct file_operations drm_connector_fops = {
>  	.write = connector_write
>  };
>  
> -#define HDMI_MAX_INFOFRAME_SIZE		29
> -
>  static ssize_t
>  audio_infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
>  {
> @@ -579,7 +577,7 @@ static ssize_t _f##_read_infoframe(struct file *filp, \
>  	struct drm_connector *connector; \
>  	union hdmi_infoframe *frame; \
>  	struct drm_device *dev; \
> -	u8 buf[HDMI_MAX_INFOFRAME_SIZE]; \
> +	u8 buf[HDMI_INFOFRAME_SIZE(MAX)]; \
>  	ssize_t len = 0; \
>  	\
>  	connector = filp->private_data; \
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 3bb87bf6bc65..3a442a59919e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -59,6 +59,9 @@ enum hdmi_infoframe_type {
>  #define HDMI_DRM_INFOFRAME_SIZE    26
>  #define HDMI_VENDOR_INFOFRAME_SIZE  4
>  
> +/* The biggest infoframe size */
> +#define HDMI_MAX_INFOFRAME_SIZE		HDMI_DRM_INFOFRAME_SIZE

Thanks for that patch, it was definitely an oversight on my part. The
spec defines the max size of an infoframe to be 30 bytes, so we should
probably use that here?

Maxime

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

Re: [PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[Jani Nikula]

On Tue, 27 Aug 2024, Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Mon, Aug 26, 2024 at 07:10:11AM GMT, Derek Foreman wrote:
>> The largest infoframe we create is the DRM (Dynamic Range Mastering)
>> infoframe which is 26 bytes + a 4 byte header, for a total of 30
>> bytes.
>> 
>> With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we
>> allocate too little space to pack a DRM infoframe in
>> write_device_infoframe(), leading to an ENOSPC return from
>> hdmi_infoframe_pack(), and never calling the connector's
>> write_infoframe() vfunc.
>> 
>> Instead of having HDMI_MAX_INFOFRAME_SIZE defined in two places,
>> replace HDMI_MAX_INFOFRAME_SIZE with HDMI_INFOFRAME_SIZE(MAX) and make
>> MAX the same size as the DRM infoframe.
>> 
>> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 +---
>>  drivers/gpu/drm/drm_debugfs.c                   | 4 +---
>>  include/linux/hdmi.h                            | 3 +++
>>  3 files changed, 5 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 7854820089ec..feb7a3a75981 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -521,8 +521,6 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
>>  
>> -#define HDMI_MAX_INFOFRAME_SIZE		29
>> -
>>  static int clear_device_infoframe(struct drm_connector *connector,
>>  				  enum hdmi_infoframe_type type)
>>  {
>> @@ -563,7 +561,7 @@ static int write_device_infoframe(struct drm_connector *connector,
>>  {
>>  	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
>>  	struct drm_device *dev = connector->dev;
>> -	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
>> +	u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
>>  	int ret;
>>  	int len;
>>  
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 6b239a24f1df..9d3e6dd68810 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -520,8 +520,6 @@ static const struct file_operations drm_connector_fops = {
>>  	.write = connector_write
>>  };
>>  
>> -#define HDMI_MAX_INFOFRAME_SIZE		29
>> -
>>  static ssize_t
>>  audio_infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
>>  {
>> @@ -579,7 +577,7 @@ static ssize_t _f##_read_infoframe(struct file *filp, \
>>  	struct drm_connector *connector; \
>>  	union hdmi_infoframe *frame; \
>>  	struct drm_device *dev; \
>> -	u8 buf[HDMI_MAX_INFOFRAME_SIZE]; \
>> +	u8 buf[HDMI_INFOFRAME_SIZE(MAX)]; \
>>  	ssize_t len = 0; \
>>  	\
>>  	connector = filp->private_data; \
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
>> index 3bb87bf6bc65..3a442a59919e 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -59,6 +59,9 @@ enum hdmi_infoframe_type {
>>  #define HDMI_DRM_INFOFRAME_SIZE    26
>>  #define HDMI_VENDOR_INFOFRAME_SIZE  4
>>  
>> +/* The biggest infoframe size */
>> +#define HDMI_MAX_INFOFRAME_SIZE		HDMI_DRM_INFOFRAME_SIZE
>
> Thanks for that patch, it was definitely an oversight on my part. The
> spec defines the max size of an infoframe to be 30 bytes, so we should
> probably use that here?

Defining it like this allows the use of HDMI_INFOFRAME_SIZE(MAX) which
adds the header size. Having some macros include the header size and
some others not would be confusing, I think.

Or then the whole thing needs to be renamed.

BR,
Jani.

>
> Maxime

-- 
Jani Nikula, Intel

Re: [PATCH v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes

[Derek Foreman]

On 2024-08-27 05:19, Jani Nikula wrote:
> On Tue, 27 Aug 2024, Maxime Ripard <mripard@kernel.org> wrote:
>> Hi,
>>
>> On Mon, Aug 26, 2024 at 07:10:11AM GMT, Derek Foreman wrote:
>>> The largest infoframe we create is the DRM (Dynamic Range Mastering)
>>> infoframe which is 26 bytes + a 4 byte header, for a total of 30
>>> bytes.
>>>
>>> With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we
>>> allocate too little space to pack a DRM infoframe in
>>> write_device_infoframe(), leading to an ENOSPC return from
>>> hdmi_infoframe_pack(), and never calling the connector's
>>> write_infoframe() vfunc.
>>>
>>> Instead of having HDMI_MAX_INFOFRAME_SIZE defined in two places,
>>> replace HDMI_MAX_INFOFRAME_SIZE with HDMI_INFOFRAME_SIZE(MAX) and make
>>> MAX the same size as the DRM infoframe.
>>>
>>> Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
>>> ---
>>>   drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 +---
>>>   drivers/gpu/drm/drm_debugfs.c                   | 4 +---
>>>   include/linux/hdmi.h                            | 3 +++
>>>   3 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>> index 7854820089ec..feb7a3a75981 100644
>>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>> @@ -521,8 +521,6 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>>>   }
>>>   EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check);
>>>   
>>> -#define HDMI_MAX_INFOFRAME_SIZE		29
>>> -
>>>   static int clear_device_infoframe(struct drm_connector *connector,
>>>   				  enum hdmi_infoframe_type type)
>>>   {
>>> @@ -563,7 +561,7 @@ static int write_device_infoframe(struct drm_connector *connector,
>>>   {
>>>   	const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
>>>   	struct drm_device *dev = connector->dev;
>>> -	u8 buffer[HDMI_MAX_INFOFRAME_SIZE];
>>> +	u8 buffer[HDMI_INFOFRAME_SIZE(MAX)];
>>>   	int ret;
>>>   	int len;
>>>   
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>>> index 6b239a24f1df..9d3e6dd68810 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -520,8 +520,6 @@ static const struct file_operations drm_connector_fops = {
>>>   	.write = connector_write
>>>   };
>>>   
>>> -#define HDMI_MAX_INFOFRAME_SIZE		29
>>> -
>>>   static ssize_t
>>>   audio_infoframe_read(struct file *filp, char __user *ubuf, size_t count, loff_t *ppos)
>>>   {
>>> @@ -579,7 +577,7 @@ static ssize_t _f##_read_infoframe(struct file *filp, \
>>>   	struct drm_connector *connector; \
>>>   	union hdmi_infoframe *frame; \
>>>   	struct drm_device *dev; \
>>> -	u8 buf[HDMI_MAX_INFOFRAME_SIZE]; \
>>> +	u8 buf[HDMI_INFOFRAME_SIZE(MAX)]; \
>>>   	ssize_t len = 0; \
>>>   	\
>>>   	connector = filp->private_data; \
>>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
>>> index 3bb87bf6bc65..3a442a59919e 100644
>>> --- a/include/linux/hdmi.h
>>> +++ b/include/linux/hdmi.h
>>> @@ -59,6 +59,9 @@ enum hdmi_infoframe_type {
>>>   #define HDMI_DRM_INFOFRAME_SIZE    26
>>>   #define HDMI_VENDOR_INFOFRAME_SIZE  4
>>>   
>>> +/* The biggest infoframe size */
>>> +#define HDMI_MAX_INFOFRAME_SIZE		HDMI_DRM_INFOFRAME_SIZE
>> Thanks for that patch, it was definitely an oversight on my part. The
>> spec defines the max size of an infoframe to be 30 bytes, so we should
>> probably use that here?

Hmm, I think the spec (I'm looking at 1.3a, which I think is the latest 
freely available version from hdmi.org?) says a maximum length of 27 
bytes, not including the 3 byte header + the checksum byte. So I think 31?

I'm seeing this in section 5.3.5 / Table 5-14.

> Defining it like this allows the use of HDMI_INFOFRAME_SIZE(MAX) which
> adds the header size. Having some macros include the header size and
> some others not would be confusing, I think.
>
> Or then the whole thing needs to be renamed.

How about:

#define HDMI_MAX_INFOFRAME_SIZE 27

Then we can use HDMI_DRM_INFOFRAME_SIZE(MAX) to get 31

Would that be ok?

Thanks,
Derek

> BR,
> Jani.
>
>> Maxime