Hi Maxime, Thank you for the patch.
On Mon, Sep 07, 2026 at 03:23:40PM +0200, Maxime Ripard wrote: > drm_private_obj does not carry a human-readable name, which makes > debug messages hard to interpret when multiple private objects are > registered on the same device. > > Add a name field to drm_private_obj, passed through > drm_atomic_private_obj_init() and freed in > drm_atomic_private_obj_fini(). Update the existing callers to pass a > name. > > Signed-off-by: Maxime Ripard <[email protected]> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 + > drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c | 8 ++++++++ > drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +- > drivers/gpu/drm/display/drm_dp_tunnel.c | 1 + > drivers/gpu/drm/drm_atomic.c | 4 ++++ > drivers/gpu/drm/drm_bridge.c | 3 ++- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 1 + > drivers/gpu/drm/ingenic/ingenic-ipu.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > drivers/gpu/drm/omapdrm/omap_drv.c | 1 + > drivers/gpu/drm/tegra/hub.c | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 3 +++ > include/drm/drm_atomic.h | 7 ++++++- > include/drm/drm_bridge.h | 2 ++ > 15 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index a9d3ce8b173a..1d481c9437d7 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -2249,10 +2249,11 @@ static int amdgpu_dm_mode_config_init(struct > amdgpu_device *adev) > /* indicates support for immediate flip */ > adev_to_drm(adev)->mode_config.async_page_flip = true; > > drm_atomic_private_obj_init(adev_to_drm(adev), > &adev->dm.atomic_obj, > + "amdgpu_dc_state", I'll let individual driver maintainers comment no the names. > &dm_atomic_state_funcs); > > r = amdgpu_display_modeset_create_props(adev); > if (r) > return r; > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > index 77b3f361091f..8a3c9fcf3844 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_private_obj.c > @@ -64,10 +64,11 @@ static const struct drm_private_state_funcs > komeda_layer_obj_funcs = { > > static int komeda_layer_obj_add(struct komeda_kms_dev *kms, > struct komeda_layer *layer) > { > drm_atomic_private_obj_init(&kms->base, &layer->base.obj, > + "komeda_layer", > &komeda_layer_obj_funcs); > return 0; > } > > static struct drm_private_state * > @@ -117,10 +118,11 @@ static const struct drm_private_state_funcs > komeda_scaler_obj_funcs = { > static int komeda_scaler_obj_add(struct komeda_kms_dev *kms, > struct komeda_scaler *scaler) > { > drm_atomic_private_obj_init(&kms->base, > &scaler->base.obj, > + "komeda_scaler", > &komeda_scaler_obj_funcs); > return 0; > } > > static struct drm_private_state * > @@ -169,10 +171,11 @@ static const struct drm_private_state_funcs > komeda_compiz_obj_funcs = { > > static int komeda_compiz_obj_add(struct komeda_kms_dev *kms, > struct komeda_compiz *compiz) > { > drm_atomic_private_obj_init(&kms->base, &compiz->base.obj, > + "komeda_compiz", > &komeda_compiz_obj_funcs); > > return 0; > } > > @@ -223,10 +226,11 @@ static const struct drm_private_state_funcs > komeda_splitter_obj_funcs = { > static int komeda_splitter_obj_add(struct komeda_kms_dev *kms, > struct komeda_splitter *splitter) > { > drm_atomic_private_obj_init(&kms->base, > &splitter->base.obj, > + "komeda_splitter", > &komeda_splitter_obj_funcs); > > return 0; > } > > @@ -276,10 +280,11 @@ static const struct drm_private_state_funcs > komeda_merger_obj_funcs = { > static int komeda_merger_obj_add(struct komeda_kms_dev *kms, > struct komeda_merger *merger) > { > drm_atomic_private_obj_init(&kms->base, > &merger->base.obj, > + "komeda_merger", > &komeda_merger_obj_funcs); > > return 0; > } > > @@ -329,10 +334,11 @@ static const struct drm_private_state_funcs > komeda_improc_obj_funcs = { > > static int komeda_improc_obj_add(struct komeda_kms_dev *kms, > struct komeda_improc *improc) > { > drm_atomic_private_obj_init(&kms->base, &improc->base.obj, > + "komeda_improc", > &komeda_improc_obj_funcs); > > return 0; > } > > @@ -382,10 +388,11 @@ static const struct drm_private_state_funcs > komeda_timing_ctrlr_obj_funcs = { > > static int komeda_timing_ctrlr_obj_add(struct komeda_kms_dev *kms, > struct komeda_timing_ctrlr *ctrlr) > { > drm_atomic_private_obj_init(&kms->base, &ctrlr->base.obj, > + "komeda_timing_ctrlr", > &komeda_timing_ctrlr_obj_funcs); > > return 0; > } > > @@ -436,10 +443,11 @@ static const struct drm_private_state_funcs > komeda_pipeline_obj_funcs = { > > static int komeda_pipeline_obj_add(struct komeda_kms_dev *kms, > struct komeda_pipeline *pipe) > { > drm_atomic_private_obj_init(&kms->base, &pipe->obj, > + "komeda_pipeline", > &komeda_pipeline_obj_funcs); > > return 0; > } > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 7ce9e212770a..d3ed80d9fce3 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -5766,11 +5766,11 @@ int drm_dp_mst_topology_mgr_init(struct > drm_dp_mst_topology_mgr *mgr, > mgr->aux = aux; > mgr->max_dpcd_transaction_bytes = max_dpcd_transaction_bytes; > mgr->max_payloads = max_payloads; > mgr->conn_base_id = conn_base_id; > > - drm_atomic_private_obj_init(dev, &mgr->base, > + drm_atomic_private_obj_init(dev, &mgr->base, "drm_dp_mst_topology", > &drm_dp_mst_topology_state_funcs); > > return 0; > } > EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init); > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > b/drivers/gpu/drm/display/drm_dp_tunnel.c > index d29ad4116e64..391d643793eb 100644 > --- a/drivers/gpu/drm/display/drm_dp_tunnel.c > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -1730,10 +1730,11 @@ static bool init_group(struct drm_dp_tunnel_mgr *mgr, > struct drm_dp_tunnel_group > group->mgr = mgr; > group->available_bw = -1; > INIT_LIST_HEAD(&group->tunnels); > > drm_atomic_private_obj_init(mgr->dev, &group->base, > + group->name, > &tunnel_group_funcs); > > return true; > } > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 596b0de44f00..04da72b4aa70 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1006,10 +1006,11 @@ static void drm_atomic_plane_print_state(struct > drm_printer *p, > > /** > * drm_atomic_private_obj_init - initialize private object > * @dev: DRM device this object will be attached to > * @obj: private object > + * @name: human-readable name for debug messages > * @funcs: pointer to the struct of function pointers that identify the > object > * type > * > * Initialize the private object, which can be embedded into any > * driver private object that needs its own atomic state. > @@ -1017,18 +1018,20 @@ static void drm_atomic_plane_print_state(struct > drm_printer *p, > * RETURNS: > * Zero on success, error code on failure > */ > int drm_atomic_private_obj_init(struct drm_device *dev, > struct drm_private_obj *obj, > + const char *name, > const struct drm_private_state_funcs *funcs) > { > struct drm_private_state *state; > memset(obj, 0, sizeof(*obj)); > > drm_modeset_lock_init(&obj->lock); > > obj->dev = dev; > + obj->name = kstrdup(name, GFP_KERNEL); > obj->funcs = funcs; > list_add_tail(&obj->head, &dev->mode_config.privobj_list); > > state = obj->funcs->atomic_create_state(obj); > if (IS_ERR(state)) You seem to be leaking obj->name in the error path. Actually, unless callers of drm_atomic_private_obj_init() are required to call drm_atomic_private_obj_fini() on failure (which wouldn't be a great API), the function is already failing to remove the object from the privobj_list, and leaks the object lock. A quick grep shows that the komeda driver seems to call drm_atomic_private_obj_fini() on private objects that failed to initialized, because it iterates over the privobj_list. Other drivers may implement different patterns. It may be safer to make drm_atomic_private_obj_fini() idempotent. > @@ -1049,10 +1052,11 @@ EXPORT_SYMBOL(drm_atomic_private_obj_init); > void > drm_atomic_private_obj_fini(struct drm_private_obj *obj) > { > list_del(&obj->head); > obj->funcs->atomic_destroy_state(obj, obj->state); > + kfree(obj->name); > drm_modeset_lock_fini(&obj->lock); > } > EXPORT_SYMBOL(drm_atomic_private_obj_fini); > > /** > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 83f1809a5d37..27bb9963c234 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -419,10 +419,11 @@ void *__devm_drm_bridge_alloc(struct device *dev, > size_t size, size_t offset, > > bridge = container + offset; > INIT_LIST_HEAD(&bridge->list); > bridge->container = container; > bridge->funcs = funcs; > + bridge->name = devm_kasprintf(dev, GFP_KERNEL, "bridge-%s", > dev_name(dev)); > kref_init(&bridge->refcount); > > err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge); > if (err) > return ERR_PTR(err); > @@ -622,11 +623,11 @@ int drm_bridge_attach(struct drm_encoder *encoder, > struct drm_bridge *bridge, > ret = bridge->funcs->attach(bridge, encoder, flags); > if (ret < 0) > goto err_reset_bridge; > } > > - drm_atomic_private_obj_init(bridge->dev, &bridge->base, > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, bridge->name, As far as I can see, bridge->name is used here only. We could avoid the permanent allocation by constructing the name string on-demand here, but I suppose the runtime overhead would be worse than the small saving in permanent memory consumption. > &drm_bridge_priv_state_funcs); > > return 0; > > err_reset_bridge: > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 625ec76cf04d..05ee0ccb121d 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -1400,10 +1400,11 @@ static int ingenic_drm_bind(struct device *dev, bool > has_components) > dev_err(dev, "Unable to register clock notifier\n"); > goto err_devclk_disable; > } > > drm_atomic_private_obj_init(drm, &priv->private_obj, > + "ingenic_drm", > &ingenic_drm_private_state_funcs); > > ret = drmm_add_action_or_reset(drm, ingenic_drm_atomic_private_obj_fini, > &priv->private_obj); > if (ret) > diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c > b/drivers/gpu/drm/ingenic/ingenic-ipu.c > index bf24882e8d9f..795f6db5ce59 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c > +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c > @@ -900,10 +900,11 @@ static int ingenic_ipu_bind(struct device *dev, struct > device *master, void *d) > dev_err(dev, "Unable to prepare clock\n"); > return err; > } > > drm_atomic_private_obj_init(drm, &ipu->private_obj, > + "ingenic_ipu_state", > &ingenic_ipu_private_state_funcs); > > return 0; > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index da3556eb6ecc..fb3a0c847a9e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1153,10 +1153,11 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > > dev->mode_config.cursor_width = 512; > dev->mode_config.cursor_height = 512; > > drm_atomic_private_obj_init(dpu_kms->dev, &dpu_kms->global_state, > + "dpu_kms_global_state", > &dpu_kms_global_state_funcs); > > atomic_set(&dpu_kms->bandwidth_ref, 0); > > rc = pm_runtime_resume_and_get(&dpu_kms->pdev->dev); > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index 3934cd060b27..b53f59f25fb0 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -710,10 +710,11 @@ static int mdp5_init(struct platform_device *pdev, > struct drm_device *dev) > int ret; > > mdp5_kms->dev = dev; > > drm_atomic_private_obj_init(mdp5_kms->dev, &mdp5_kms->glob_state, > + "mdp5_global_state", > &mdp5_global_state_funcs); > > /* we need to set a default rate before enabling. Set a safe > * rate first, then figure out hw revision, and then set a > * more optimal rate: > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c > index 92d6a1f9c0a1..9738acbf6fae 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -298,10 +298,11 @@ static const struct drm_private_state_funcs > omap_global_state_funcs = { > static int omap_global_obj_init(struct drm_device *dev) > { > struct omap_drm_private *priv = dev->dev_private; > > drm_atomic_private_obj_init(dev, &priv->glob_obj, > + "omap_global", > &omap_global_state_funcs); > return 0; > } > > static void omap_global_obj_fini(struct omap_drm_private *priv) > diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c > index bd442bfd4540..b38fd353a648 100644 > --- a/drivers/gpu/drm/tegra/hub.c > +++ b/drivers/gpu/drm/tegra/hub.c > @@ -956,10 +956,11 @@ static int tegra_display_hub_init(struct host1x_client > *client) > struct tegra_display_hub *hub = to_tegra_display_hub(client); > struct drm_device *drm = dev_get_drvdata(client->host); > struct tegra_drm *tegra = drm->dev_private; > > drm_atomic_private_obj_init(drm, &hub->base, > + "tegra_display_hub", > &tegra_display_hub_state_funcs); > > tegra->hub = hub; > > return 0; > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index b17e73bce384..5f64cf1fcc9e 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -115,10 +115,11 @@ static void vc4_ctm_obj_fini(struct drm_device *dev, > void *unused) > static int vc4_ctm_obj_init(struct vc4_dev *vc4) > { > drm_modeset_lock_init(&vc4->ctm_state_lock); > > drm_atomic_private_obj_init(&vc4->base, &vc4->ctm_manager, > + "vc4_ctm", > &vc4_ctm_state_funcs); > > return drmm_add_action_or_reset(&vc4->base, vc4_ctm_obj_fini, NULL); > } > > @@ -755,10 +756,11 @@ static void vc4_load_tracker_obj_fini(struct drm_device > *dev, void *unused) > } > > static int vc4_load_tracker_obj_init(struct vc4_dev *vc4) > { > drm_atomic_private_obj_init(&vc4->base, &vc4->load_tracker, > + "vc4_load_tracker", > &vc4_load_tracker_state_funcs); > > return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, > NULL); > } > > @@ -846,10 +848,11 @@ static void vc4_hvs_channels_obj_fini(struct drm_device > *dev, void *unused) > } > > static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) > { > drm_atomic_private_obj_init(&vc4->base, &vc4->hvs_channels, > + "vc4_hvs_channels", > &vc4_hvs_state_funcs); > > return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, > NULL); > } > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index def18b60a106..47b2fe2b039e 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -357,10 +357,15 @@ struct drm_private_obj { > /** > * @dev: parent DRM device > */ > struct drm_device *dev; > > + /** > + * @name: human-readable name for debug messages > + */ > + const char *name; > + > /** > * @head: List entry used to attach a private object to a &drm_device > * (queued to &drm_mode_config.privobj_list). > */ > struct list_head head; > @@ -738,11 +743,11 @@ drm_atomic_get_new_colorop_state(struct > drm_atomic_commit *state, > struct drm_connector_state * __must_check > drm_atomic_get_connector_state(struct drm_atomic_commit *state, > struct drm_connector *connector); > > int drm_atomic_private_obj_init(struct drm_device *dev, > - struct drm_private_obj *obj, > + struct drm_private_obj *obj, const char *name, > const struct drm_private_state_funcs *funcs); > void drm_atomic_private_obj_fini(struct drm_private_obj *obj); > > struct drm_private_state * __must_check > drm_atomic_get_private_obj_state(struct drm_atomic_commit *state, > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 58fff047f43b..389dcab3c3f3 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -994,10 +994,12 @@ enum drm_bridge_ops { > struct drm_bridge { > /** @base: inherit from &drm_private_object */ > struct drm_private_obj base; > /** @dev: DRM device this bridge belongs to */ > struct drm_device *dev; > + /** @name: human-readable name for debug messages */ > + const char *name; > /** @encoder: encoder to which this bridge is connected */ > struct drm_encoder *encoder; > /** @chain_node: used to form a bridge chain */ > struct list_head chain_node; > /** @of_node: device node pointer to the bridge */ -- Regards, Laurent Pinchart
