Switch drm_bridge_connector to the new drmm_connector_hdmi_init()
signature. The vendor, product, supported_formats and max_bpc values
now live in struct drm_connector_hdmi_funcs instead of being passed as
separate arguments. This allows future HDMI 2.x capabilities to be
plumbed without growing the function's argument list.
Introduce the supported_hdmi_ver field in struct drm_bridge to let
bridges declare the HDMI specification version they comply with,
as well as max_tmds_char_rate to allow them to override the limit
inferred from that version. This is particularly useful when handling
chip variants with different capabilities.
Additionally, add a warning in drm_bridge_add() if an HDMI bridge leaves
supported_hdmi_ver unset. This alerts bridge drivers setting the
DRM_BRIDGE_OP_HDMI flag that they must also declare their supported HDMI
version.
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
drivers/gpu/drm/display/drm_bridge_connector.c | 43 ++++++++++++++++----------
drivers/gpu/drm/drm_bridge.c | 6 +++-
include/drm/drm_bridge.h | 14 +++++++++
3 files changed, 46 insertions(+), 17 deletions(-)
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless removal of bridge from `bridge_lingering_list` can corrupt the global list.
--
--- Patch [4]: [PATCH v9 04/61] drm/display: bridge_connector: Convert to drmm_connector_hdmi_init() ---
commit d79fbb96aa85834c574a6211898049f6cf97c1a6
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/display: bridge_connector: Convert to drmm_connector_hdmi_init()
Switch drm_bridge_connector to the new drmm_connector_hdmi_init()
signature. The vendor, product, supported_formats and max_bpc values
now live in struct drm_connector_hdmi_funcs instead of being passed as
separate arguments. Introduce supported_hdmi_ver and max_tmds_char_rate to
struct drm_bridge to let bridges declare their capabilities.
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 83f1809a5d376..afaae272347c0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -460,9 +460,13 @@ void drm_bridge_add(struct drm_bridge *bridge)
[Severity: High]
This is a pre-existing issue, but looking at drm_bridge_add() just before
this hunk, does the lockless list removal risk corrupting the global
bridge_lingering_list?
It looks like the bridge is being removed from the list without acquiring
bridge_lock:
if (!list_empty(&bridge->list))
list_del_init(&bridge->list);
If another thread concurrently modifies bridge_lingering_list via
drm_bridge_remove(), or iterates over it via debugfs, couldn't this
lead to list pointer corruption or a crash?
> mutex_init(&bridge->hpd_state_mutex);
> mutex_init(&bridge->hpd_mutex);
>
> - if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> + if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
> + if (bridge->supported_hdmi_ver == HDMI_VERSION_UNKNOWN)
> + DRM_WARN("HDMI bridge misses supported HDMI version\n");
> +
> bridge->ycbcr_420_allowed = !!(bridge->supported_formats &
> BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420));
> + }
>
> mutex_lock(&bridge_lock);
> list_add_tail(&bridge->list, &bridge_list);
@@ -590,6 +590,8 @@ drm_bridge_connector_read_edid(struct drm_connector *connector)
}
static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
+ .supported_formats = BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444),
+ .max_bpc = 8,
.tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
.read_edid = drm_bridge_connector_read_edid,
.avi = {
@@ -823,8 +825,6 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
struct drm_connector *connector;
struct i2c_adapter *ddc = NULL;
struct drm_bridge *panel_bridge __free(drm_bridge_put) = NULL;
- unsigned int supported_formats = BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444);
- unsigned int max_bpc = 8;
bool support_hdcp = false;
int connector_type;
int ret;
@@ -907,11 +907,6 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(-EINVAL);
bridge_connector->bridge_hdmi = drm_bridge_get(bridge);
-
- if (bridge->supported_formats)
- supported_formats = bridge->supported_formats;
- if (bridge->max_bpc)
- max_bpc = bridge->max_bpc;
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
@@ -993,11 +988,31 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(-EINVAL);
if (bridge_connector->bridge_hdmi) {
- if (!connector->ycbcr_420_allowed)
- supported_formats &= ~BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420);
-
bridge_connector->hdmi_funcs = drm_bridge_connector_hdmi_funcs;
+ bridge_connector->hdmi_funcs.vendor = bridge_connector->bridge_hdmi->vendor;
+ bridge_connector->hdmi_funcs.product = bridge_connector->bridge_hdmi->product;
+
+ if (bridge_connector->bridge_hdmi->supported_hdmi_ver)
+ bridge_connector->hdmi_funcs.supported_hdmi_ver =
+ bridge_connector->bridge_hdmi->supported_hdmi_ver;
+
+ if (bridge_connector->bridge_hdmi->supported_formats)
+ bridge_connector->hdmi_funcs.supported_formats =
+ bridge_connector->bridge_hdmi->supported_formats;
+
+ if (!connector->ycbcr_420_allowed)
+ bridge_connector->hdmi_funcs.supported_formats &=
+ ~BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420);
+
+ if (bridge_connector->bridge_hdmi->max_tmds_char_rate)
+ bridge_connector->hdmi_funcs.supported_tmds_char_rate =
+ bridge_connector->bridge_hdmi->max_tmds_char_rate;
+
+ if (bridge_connector->bridge_hdmi->max_bpc)
+ bridge_connector->hdmi_funcs.max_bpc =
+ bridge_connector->bridge_hdmi->max_bpc;
+
if (bridge_connector->bridge_hdmi->ops & DRM_BRIDGE_OP_HDMI_AUDIO)
bridge_connector->hdmi_funcs.audio =
drm_bridge_connector_hdmi_audio_infoframe;
@@ -1010,14 +1025,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
bridge_connector->hdmi_funcs.spd =
drm_bridge_connector_hdmi_spd_infoframe;
- ret = drmm_connector_hdmi_ini2(drm, connector,
- bridge_connector->bridge_hdmi->vendor,
- bridge_connector->bridge_hdmi->product,
+ ret = drmm_connector_hdmi_init(drm, connector,
&drm_bridge_connector_funcs,
&bridge_connector->hdmi_funcs,
- connector_type, ddc,
- supported_formats,
- max_bpc);
+ connector_type, ddc);
if (ret)
return ERR_PTR(ret);
} else {
@@ -460,9 +460,13 @@ void drm_bridge_add(struct drm_bridge *bridge)
mutex_init(&bridge->hpd_state_mutex);
mutex_init(&bridge->hpd_mutex);
- if (bridge->ops & DRM_BRIDGE_OP_HDMI)
+ if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
+ if (bridge->supported_hdmi_ver == HDMI_VERSION_UNKNOWN)
+ DRM_WARN("HDMI bridge misses supported HDMI version\n");
+
bridge->ycbcr_420_allowed = !!(bridge->supported_formats &
BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420));
+ }
mutex_lock(&bridge_lock);
list_add_tail(&bridge->list, &bridge_list);
@@ -1081,6 +1081,12 @@ struct drm_bridge {
*/
const char *product;
+ /**
+ * @supported_hdmi_ver: HDMI version the bridge is conformant with.
+ * This is only relevant if @DRM_BRIDGE_OP_HDMI is set.
+ */
+ enum hdmi_version supported_hdmi_ver;
+
/**
* @supported_formats: Bitmask of @drm_output_color_format listing
* supported output formats. This is only relevant if
@@ -1088,6 +1094,14 @@ struct drm_bridge {
*/
unsigned int supported_formats;
+ /**
+ * @max_tmds_char_rate: Maximum TMDS character rate, in Hz, the HDMI
+ * bridge supports. A value of 0 means the core should use the default
+ * limit implied by @supported_hdmi_ver. This is only relevant if
+ * @DRM_BRIDGE_OP_HDMI is set.
+ */
+ unsigned long long max_tmds_char_rate;
+
/**
* @max_bpc: Maximum bits per char the HDMI bridge supports. Allowed
* values are 8, 10 and 12. This is only relevant if