In preparation for providing HDMI 2.x source capabilities, move the
vendor, product, supported_formats and max_bpc parameters out of the
init function into struct drm_connector_hdmi_funcs, and add new
supported_hdmi_ver and supported_tmds_char_rate fields there as well.
Appending more HDMI-specific arguments to that function would not scale
well, hence introduce drmm_connector_hdmi_init() with the reduced
signature.
Additionally, add the max_tmds_char_rate field to struct
drm_connector_hdmi and use the supported HDMI version to determinate the
maximum TMDS character rate allowed by the specification. Some
controllers, however, may support a lower rate than that version would
imply. A non-zero supported_tmds_char_rate lets drivers override this
default with the actual controller capability. A value of zero keeps
the limit inferred from supported_hdmi_ver.
Callers are converted in the following patches, while the old and now
renamed *_ini2() helper is removed at the end of the series.
No functional changes expected for existing callers.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
drivers/gpu/drm/drm_connector.c | 134 ++++++++++++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 62 ++++++++++++++++++-
2 files changed, 195 insertions(+), 1 deletion(-)
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Missing validation for `hdmi_funcs->supported_hdmi_ver` leading to a 0 Hz TMDS rate limit or spurious -EINVAL on initialization.
Pre-existing issues:
- [High] Missing NULL check on `connector->state` before calling `drm_connector_attach_max_bpc_property()`, leading to a potential NULL pointer dereference.
--
--- Patch [3]: [PATCH v9 03/61] drm/connector: Add drmm_connector_hdmi_init() with new signature ---
commit 68ee3a0c1e1945b6e973060559a1cafa855a35d7
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Subject: drm/connector: Add drmm_connector_hdmi_init() with new signature
This patch introduces a new function drmm_connector_hdmi_init() with a
reduced signature by moving HDMI-specific parameters into struct
drm_connector_hdmi_funcs. It also infers the maximum TMDS character rate
from the supported HDMI version or allows an override if specified.
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
[ ... ]
> + /*
> + * The supported HDMI version can be used to determinate the maximum
> + * TMDS character rate allowed by the specification. Some controllers,
> + * however, may support a lower rate than that version would imply.
> + *
> + * A non-zero caps->max_tmds_char_rate lets drivers override this
> + * default with the actual controller capability. A value of zero keeps
> + * the limit inferred from supported_hdmi_ver.
> + */
> + if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
> + connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> + else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> + connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> + else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> + connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
[Severity: High]
What happens if hdmi_funcs->supported_hdmi_ver is 0 (HDMI_VERSION_UNKNOWN) in
drmm_connector_hdmi_init()?
If a driver developer forgets to initialize hdmi_funcs->supported_hdmi_ver,
it defaults to 0. This if/else block falls through without setting
max_tmds_char_rate, leaving it at 0.
If a driver provides a custom supported_tmds_char_rate, initialization will
then fail with -EINVAL. Otherwise, the connector initializes with a 0 Hz
TMDS limit, which could break display mode validation by rejecting all
display modes.
> +
> + if (hdmi_funcs->supported_tmds_char_rate) {
> + if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
> + drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
> + connector->hdmi.max_tmds_char_rate);
> + return -EINVAL;
> + }
> + connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
> + }
[ ... ]
> + if (connector->funcs->atomic_create_state) {
> + struct drm_connector_state *state;
> +
> + state = connector->funcs->atomic_create_state(connector);
> + if (IS_ERR(state))
> + return PTR_ERR(state);
> +
> + connector->state = state;
> + } else if (connector->funcs->reset) {
> + connector->funcs->reset(connector);
> + }
> +
> + drm_connector_attach_max_bpc_property(connector, 8, hdmi_funcs->max_bpc);
[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference in drm_connector_attach_max_bpc_property() if the reset
callback fails?
In drmm_connector_hdmi_init(), if the reset callback fails to allocate memory
(e.g., inside drm_atomic_helper_connector_reset()), connector->state will
remain NULL.
When this is unconditionally passed to
drm_connector_attach_max_bpc_property(), would it dereference the NULL state:
drivers/gpu/drm/drm_connector.c:drm_connector_attach_max_bpc_property() {
...
connector->state->max_requested_bpc = max;
...
}
@@ -542,6 +542,140 @@ int drmm_connector_init(struct drm_device *dev,
}
EXPORT_SYMBOL(drmm_connector_init);
+/**
+ * drmm_connector_hdmi_init - Init a preallocated HDMI connector
+ * @dev: DRM device
+ * @connector: A pointer to the HDMI connector to init
+ * @funcs: callbacks for this connector
+ * @hdmi_funcs: HDMI-related callbacks and capabilities for this connector
+ * @connector_type: user visible type of the connector
+ * @ddc: optional pointer to the associated ddc adapter
+ *
+ * Initialises a preallocated HDMI connector. Connectors can be
+ * subclassed as part of driver connector objects.
+ *
+ * Cleanup is automatically handled with a call to
+ * drm_connector_cleanup() in a DRM-managed action.
+ *
+ * The connector structure should be allocated with drmm_kzalloc().
+ *
+ * The @drm_connector_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drmm_connector_hdmi_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ const struct drm_connector_hdmi_funcs *hdmi_funcs,
+ int connector_type,
+ struct i2c_adapter *ddc)
+{
+ int ret;
+
+ if (!hdmi_funcs)
+ return -EINVAL;
+
+ if (!hdmi_funcs->vendor || !hdmi_funcs->product)
+ return -EINVAL;
+
+ if ((strlen(hdmi_funcs->vendor) > DRM_CONNECTOR_HDMI_VENDOR_LEN) ||
+ (strlen(hdmi_funcs->product) > DRM_CONNECTOR_HDMI_PRODUCT_LEN))
+ return -EINVAL;
+
+ if (!hdmi_funcs->supported_formats ||
+ !(hdmi_funcs->supported_formats & BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444)))
+ return -EINVAL;
+
+ if (connector->ycbcr_420_allowed !=
+ !!(hdmi_funcs->supported_formats & BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420)))
+ return -EINVAL;
+
+ if (!(hdmi_funcs->max_bpc == 8 ||
+ hdmi_funcs->max_bpc == 10 ||
+ hdmi_funcs->max_bpc == 12))
+ return -EINVAL;
+
+ if (!hdmi_funcs->avi.clear_infoframe ||
+ !hdmi_funcs->avi.write_infoframe ||
+ !hdmi_funcs->hdmi.clear_infoframe ||
+ !hdmi_funcs->hdmi.write_infoframe)
+ return -EINVAL;
+
+ if (!(connector_type == DRM_MODE_CONNECTOR_HDMIA ||
+ connector_type == DRM_MODE_CONNECTOR_HDMIB))
+ return -EINVAL;
+
+ ret = drmm_connector_init(dev, connector, funcs, connector_type, ddc);
+ if (ret)
+ return ret;
+
+ /* TODO: remove after conversion to new drmm_connector_hdmi_init() */
+ connector->hdmi.supported_formats = hdmi_funcs->supported_formats;
+
+ /*
+ * The supported HDMI version can be used to determinate the maximum
+ * TMDS character rate allowed by the specification. Some controllers,
+ * however, may support a lower rate than that version would imply.
+ *
+ * A non-zero caps->max_tmds_char_rate lets drivers override this
+ * default with the actual controller capability. A value of zero keeps
+ * the limit inferred from supported_hdmi_ver.
+ */
+ if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
+ connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
+ else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
+ connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
+ else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
+ connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
+
+ if (hdmi_funcs->supported_tmds_char_rate) {
+ if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
+ drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
+ connector->hdmi.max_tmds_char_rate);
+ return -EINVAL;
+ }
+ connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
+ }
+
+ /* TODO: remove after conversion to new drmm_connector_hdmi_init() */
+ strtomem_pad(connector->hdmi.vendor, hdmi_funcs->vendor, 0);
+ strtomem_pad(connector->hdmi.product, hdmi_funcs->product, 0);
+
+ /*
+ * drm_connector_attach_max_bpc_property() requires the
+ * connector to have a state.
+ */
+ if (connector->funcs->atomic_create_state) {
+ struct drm_connector_state *state;
+
+ state = connector->funcs->atomic_create_state(connector);
+ if (IS_ERR(state))
+ return PTR_ERR(state);
+
+ connector->state = state;
+ } else if (connector->funcs->reset) {
+ connector->funcs->reset(connector);
+ }
+
+ drm_connector_attach_max_bpc_property(connector, 8, hdmi_funcs->max_bpc);
+ /* TODO: remove after conversion to new drmm_connector_hdmi_init() */
+ connector->max_bpc = hdmi_funcs->max_bpc;
+
+ if (hdmi_funcs->max_bpc > 8)
+ drm_connector_attach_hdr_output_metadata_property(connector);
+
+ ret = drm_connector_attach_color_format_property(connector,
+ hdmi_funcs->supported_formats);
+ if (ret)
+ return ret;
+
+ connector->hdmi.funcs = hdmi_funcs;
+
+ return 0;
+}
+EXPORT_SYMBOL(drmm_connector_hdmi_init);
+
/**
* drmm_connector_hdmi_ini2 - Init a preallocated HDMI connector
* @dev: DRM device
@@ -1403,8 +1403,50 @@ struct drm_connector_infoframe_funcs {
/**
* struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions
+ * and controller capabilities
*/
struct drm_connector_hdmi_funcs {
+ /**
+ * @vendor: HDMI Controller Vendor name.
+ */
+ const char *vendor;
+
+ /**
+ * @product: HDMI Controller Product name
+ */
+ const char *product;
+
+ /**
+ * @supported_hdmi_ver:
+ *
+ * Maximum HDMI specification version supported by the controller side
+ * of this connector. This describes the controller capability only;
+ * the effective link capabilities may be further restricted by the
+ * sink, bridge, or mode validation.
+ */
+ enum hdmi_version supported_hdmi_ver;
+
+ /**
+ * @supported_tmds_char_rate:
+ *
+ * Maximum TMDS character rate supported by the controller, in Hz.
+ * A value of 0 means the core should use the default limit implied by
+ * @supported_hdmi_ver.
+ */
+ unsigned long long supported_tmds_char_rate;
+
+ /**
+ * @supported_formats:
+ *
+ * Bitmask of @drm_output_color_format listing supported output formats.
+ */
+ unsigned long supported_formats;
+
+ /**
+ * @max_bpc: Maximum bits per char the HDMI connector supports.
+ */
+ unsigned int max_bpc;
+
/**
* @tmds_char_rate_valid:
*
@@ -2052,7 +2094,19 @@ struct drm_connector_hdmi {
unsigned long supported_formats;
/**
- * @funcs: HDMI connector Control Functions
+ * @max_tmds_char_rate: Maximum TMDS character rate, in Hz,
+ * supported by the controller.
+ *
+ * This is inferred from &drm_connector_hdmi_funcs.supported_hdmi_ver,
+ * by default. However, controllers may support a lower rate than that
+ * specification version would imply. If that is the case, drivers are
+ * expected to set &drm_connector_hdmi_funcs.supported_tmds_char_rate
+ * to a non-zero value indicating the actual limit.
+ */
+ unsigned long long max_tmds_char_rate;
+
+ /**
+ * @funcs: HDMI connector Control Functions and controller capabilities
*/
const struct drm_connector_hdmi_funcs *funcs;
@@ -2549,6 +2603,12 @@ 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 struct drm_connector_funcs *funcs,
+ const struct drm_connector_hdmi_funcs *hdmi_funcs,
+ int connector_type,
+ struct i2c_adapter *ddc);
int drmm_connector_hdmi_ini2(struct drm_device *dev,
struct drm_connector *connector,
const char *vendor, const char *product,