On Wed, Oct 08, 2025 at 05:06:43PM +0300, Ville Syrjälä wrote: > On Wed, Oct 08, 2025 at 02:04:03PM +0200, Maxime Ripard wrote: > > The DP MST implementation relies on a drm_private_obj, that is > > initialized by allocating and initializing a state, and then passing it > > to drm_private_obj_init. > > > > Since we're gradually moving away from that pattern to the more > > established one relying on a reset implementation, let's migrate this > > instance to the new pattern. > > > > Signed-off-by: Maxime Ripard <[email protected]> > > --- > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 39 > > ++++++++++++++++++--------- > > 1 file changed, 26 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > index > > 64e5c176d5cce9df9314f77a0b4c97662c30c070..255fbdcea9f0b6376d15439e3da1dc02be472a20 > > 100644 > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > @@ -5181,10 +5181,34 @@ static void drm_dp_mst_destroy_state(struct > > drm_private_obj *obj, > > > > kfree(mst_state->commit_deps); > > kfree(mst_state); > > } > > > > +static void drm_dp_mst_reset(struct drm_private_obj *obj) > > +{ > > + struct drm_dp_mst_topology_mgr *mgr = > > + to_dp_mst_topology_mgr(obj); > > + struct drm_dp_mst_topology_state *mst_state; > > + > > + if (obj->state) { > > + drm_dp_mst_destroy_state(obj, obj->state); > > + obj->state = NULL; > > I'm not a big fan of this "free+reallocate without any way to handle > failures" pattern. > > Currently i915 does not do any of this, and so there are no unhandled > points of failure. But the mst and tunneling changes here force it on > i915 as well.
A very theoretical point of failure. If I'm counting right, drm_dp_mst_topology_state takes 72 bytes. Any GFP_KERNEL allocation of less than 8 pages cannot fail. So, sure, it's less satisfying to look at, but that's about it. It's just as safe and reliable. > I think for the common things it would be nicer to just implement > the reset as just that "a reset of the current state", and leave > the "let's play fast and loose with kmalloc() failures" to the > drivers that want it. I'm sorry, but I have no idea what you mean by that. It's using the exact same pattern we've been using since forever for every driver (except, apparently, i915). What would you like to change to that pattern to accomodate i915 requirements? Maxime
signature.asc
Description: PGP signature
