| Message ID | 20240826121029.491976-1-derek.foreman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes | expand |
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)
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
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
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
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)
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(-)