On Mon, Mar 11, 2024 at 03:49:48PM +0100, Maxime Ripard wrote: > Infoframes in KMS is usually handled by a bunch of low-level helpers > that require quite some boilerplate for drivers. This leads to > discrepancies with how drivers generate them, and which are actually > sent. > > Now that we have everything needed to generate them in the HDMI > connector state, we can generate them in our common logic so that > drivers can simply reuse what we precomputed. > > Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx> > --- > drivers/gpu/drm/Kconfig | 1 + > drivers/gpu/drm/drm_atomic_state_helper.c | 323 +++++++++++++++++++++ > drivers/gpu/drm/drm_connector.c | 14 + > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 1 + > drivers/gpu/drm/tests/drm_connector_test.c | 12 + > include/drm/drm_atomic_state_helper.h | 8 + > include/drm/drm_connector.h | 133 +++++++++ > 7 files changed, 492 insertions(+) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 872edb47bb53..ad9c467e20ce 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -97,10 +97,11 @@ config DRM_KUNIT_TEST > If in doubt, say "N". > > config DRM_KMS_HELPER > tristate > depends on DRM > + select DRM_DISPLAY_HDMI_HELPER > help > CRTC helpers for KMS drivers. > > config DRM_DEBUG_DP_MST_TOPOLOGY_REFS > bool "Enable refcount backtrace history in the DP MST helpers" > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index e66272c0d006..2bf53666fc9d 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -36,10 +36,12 @@ > #include <drm/drm_plane.h> > #include <drm/drm_print.h> > #include <drm/drm_vblank.h> > #include <drm/drm_writeback.h> > > +#include <drm/display/drm_hdmi_helper.h> > + > #include <linux/slab.h> > #include <linux/dma-fence.h> > > /** > * DOC: atomic state reset and initialization > @@ -912,10 +914,143 @@ hdmi_compute_config(const struct drm_connector *connector, > } > > return -EINVAL; > } > > +static int hdmi_generate_avi_infoframe(const struct drm_connector *connector, > + struct drm_connector_state *state) > +{ > + const struct drm_display_mode *mode = > + connector_state_get_mode(state); > + struct drm_connector_hdmi_infoframe *infoframe = > + &state->hdmi.infoframes.avi; > + struct hdmi_avi_infoframe *frame = > + &infoframe->data.avi; > + bool is_full_range = state->hdmi.is_full_range; > + enum hdmi_quantization_range rgb_quant_range = > + is_full_range ? HDMI_QUANTIZATION_RANGE_FULL : HDMI_QUANTIZATION_RANGE_LIMITED; > + int ret; > + > + ret = drm_hdmi_avi_infoframe_from_display_mode(frame, connector, mode); > + if (ret) > + return ret; > + > + frame->colorspace = state->hdmi.output_format; > + > + drm_hdmi_avi_infoframe_quant_range(frame, connector, mode, rgb_quant_range); drm_hdmi_avi_infoframe_quant_range() doesn't handle YCbCr currently. > + drm_hdmi_avi_infoframe_colorimetry(frame, state); > + drm_hdmi_avi_infoframe_bars(frame, state); > + > + infoframe->set = true; > + > + return 0; > +} > + <snip> > + > +#define UPDATE_INFOFRAME(c, os, ns, i) \ > + write_or_clear_infoframe(c, \ > + &(c)->hdmi.infoframes.i, \ > + &(os)->hdmi.infoframes.i, \ > + &(ns)->hdmi.infoframes.i) This macro feels like pointless obfuscation to me. <snip> > @@ -1984,20 +2063,73 @@ struct drm_connector { > > /** > * @hdmi: HDMI-related variable and properties. > */ > struct { > +#define DRM_CONNECTOR_HDMI_VENDOR_LEN 8 > + /** > + * @vendor: HDMI Controller Vendor Name > + */ > + unsigned char vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] __nonstring; > + > +#define DRM_CONNECTOR_HDMI_PRODUCT_LEN 16 > + /** > + * @product: HDMI Controller Product Name > + */ > + unsigned char product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] __nonstring; > + > /** > * @supported_formats: Bitmask of @hdmi_colorspace > * supported by the controller. > */ > unsigned long supported_formats; > > /** > * @funcs: HDMI connector Control Functions > */ > const struct drm_connector_hdmi_funcs *funcs; > + > + /** > + * @infoframes: Current Infoframes output by the connector > + */ > + struct { > + /** > + * @lock: Mutex protecting against concurrent access to > + * the infoframes, most notably between KMS and ALSA. > + */ > + struct mutex lock; > + > + /** > + * @audio: Current Audio Infoframes structure. Protected > + * by @lock. > + */ > + struct drm_connector_hdmi_infoframe audio; > + > + /** > + * @avi: Current AVI Infoframes structure. Protected by > + * @lock. > + */ > + struct drm_connector_hdmi_infoframe avi; > + > + /** > + * @hdr_drm: Current DRM (Dynamic Range and Mastering) > + * Infoframes structure. Protected by @lock. > + */ > + struct drm_connector_hdmi_infoframe hdr_drm; > + > + /** > + * @spd: Current SPD Infoframes structure. Protected by > + * @lock. > + */ > + struct drm_connector_hdmi_infoframe spd; > + > + /** > + * @vendor: Current HDMI Vendor Infoframes structure. > + * Protected by @lock. > + */ > + struct drm_connector_hdmi_infoframe hdmi; > + } infoframes; > } hdmi; What's the deal with this bloat? These are already tracked in the connector's state so this looks entirely redundant. > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > @@ -2015,10 +2147,11 @@ int drmm_connector_init(struct drm_device *dev, > const struct drm_connector_funcs *funcs, > int connector_type, > struct i2c_adapter *ddc); > int drmm_connector_hdmi_init(struct drm_device *dev, > struct drm_connector *connector, > + const char *vendor, const char *product, > const struct drm_connector_funcs *funcs, > const struct drm_connector_hdmi_funcs *hdmi_funcs, > int connector_type, > struct i2c_adapter *ddc, > unsigned long supported_formats, > > -- > 2.43.2 -- Ville Syrjälä Intel